feat(web): atlas-integration #65
Conversation
WalkthroughThis pull request involves the deletion of several serverless functions and related files that handled user authentication, settings management, and file uploads. The changes also introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AtlasProvider
participant GraphQL_API
User->>AtlasProvider: Request user authentication
AtlasProvider->>GraphQL_API: Send authentication request
GraphQL_API-->>AtlasProvider: Return access token
AtlasProvider-->>User: Provide access token
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 curate-v2 canceled.
|
✅ Deploy Preview for curate-v2 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: 22
🧹 Outside diff range and nitpick comments (42)
web/src/utils/index.ts (1)
6-9: Enhance documentation and add null safety.The implementation is clean and focused. Consider these improvements:
- Enhance the JSDoc with more details:
/** - * Checks if a string is empty or contains only whitespace. + * Checks if a string is empty or contains only whitespace characters. + * @param str - The string to check + * @returns true if the string is empty or contains only whitespace, false otherwise + * @example + * isEmpty("") // true + * isEmpty(" ") // true + * isEmpty("text") // false */
- Add null safety to prevent runtime errors:
-export const isEmpty = (str: string): boolean => str.trim() === ""; +export const isEmpty = (str: string | null | undefined): boolean => !str || str.trim() === "";web/src/hooks/useSessionStorage.ts (1)
Line range hint
3-21: Add JSDoc comments for better documentation.Consider adding JSDoc comments to document the hook's purpose, parameters, and behavior, especially noting the session-based persistence.
+/** + * Hook for managing session-scoped storage that persists data until the browser window is closed. + * @param keyName The key under which to store the value in sessionStorage + * @param defaultValue The default value to return if no value is stored + * @returns A tuple containing the stored value and a setter function + */ export function useSessionStorage<T>(keyName: string, defaultValue: T) {web/src/utils/atlas/createMessage.ts (1)
5-23: Add error handling and documentation.The function implementation is solid, but could benefit from some improvements:
- Add error handling for non-browser environments
- Add JSDoc documentation for better developer experience
- Consider making the expiration time configurable
Here's a suggested implementation:
+/** + * Creates a SIWE (Sign-In with Ethereum) message for authentication + * @param address - The Ethereum address to sign with + * @param nonce - The nonce value for the message + * @param chainId - The chain ID (defaults to DEFAULT_CHAIN) + * @param expirationMinutes - Optional: Override the default 10-minute expiration + * @returns The formatted SIWE message + * @throws Error if not in a browser environment + */ -export const createMessage = (address: `0x${string}`, nonce: string, chainId: number = DEFAULT_CHAIN) => { +export const createMessage = ( + address: `0x${string}`, + nonce: string, + chainId: number = DEFAULT_CHAIN, + expirationMinutes: number = 10 +) => { + if (typeof window === 'undefined') { + throw new Error('createMessage must be called in a browser environment'); + } + const domain = window.location.host; const origin = window.location.origin; - // signature is valid only for 10 mins - const expirationTime = new Date(Date.now() + 10 * 60 * 1000); + const expirationTime = new Date(Date.now() + expirationMinutes * 60 * 1000); const message = createSiweMessage({ domain, address, statement: "Sign In to Kleros with Ethereum.", uri: origin, version: "1", chainId, nonce, expirationTime, }); return message; };web/src/components/EnsureAuth.tsx (1)
14-28: LGTM! Clean implementation with AtlasProvider.The component has been effectively simplified by leveraging the AtlasProvider context.
Consider adding error boundaries and improving UX.
A few suggestions to enhance robustness and user experience:
- Add error boundary for cases where AtlasProvider context might be unavailable
- Consider different button text/state when wallet is not connected
Consider this enhancement:
const EnsureAuth: React.FC<IEnsureAuth> = ({ children, className }) => { const { address } = useAccount(); const { isVerified, isSigningIn, authoriseUser } = useAtlasProvider(); + + if (!address) { + return ( + <Button + text="Connect Wallet First" + disabled={true} + {...{ className }} + /> + ); + } + return isVerified ? ( children ) : ( <Button text="Sign In" onClick={authoriseUser} - disabled={isSigningIn || !address} + disabled={isSigningIn} isLoading={isSigningIn} {...{ className }} /> ); };web/src/pages/Settings/index.tsx (2)
10-16: Consider enhancing container responsiveness.The container has good responsive padding and width constraints, but could benefit from additional responsive properties.
Consider these improvements:
const Container = styled.div` width: 100%; background-color: ${({ theme }) => theme.lightBackground}; padding: ${responsiveSize(32, 80)} ${responsiveSize(24, 136)} ${responsiveSize(76, 96)}; max-width: 1780px; margin: 0 auto; + min-height: calc(100vh - 80px); /* Adjust based on your header height */ + @media (max-width: 768px) { + padding: ${responsiveSize(24, 32)} ${responsiveSize(16, 24)}; + } `;
18-26: Implement missing user settings functionality.Based on the PR objectives, this component should handle user settings management and integrate with AtlasProvider, but these aspects are missing from the current implementation.
Consider:
- Adding a
UserSettingscomponent that integrates withAtlasProviderfor user data management- Implementing settings forms for user profile management
- Adding proper loading and error states for API interactions
- Including feedback mechanisms for successful/failed operations
Would you like assistance in implementing these features?
web/src/utils/atlas/confirmEmail.ts (1)
13-22: Consider adding address validation.The type definitions accurately reflect the GraphQL schema. However, consider adding runtime validation for the address string to ensure it matches the expected format before making the API call.
+import { isAddress } from "ethers"; + export type ConfirmEmailData = { address: string; token: string; }; + +export function validateConfirmEmailData(data: ConfirmEmailData): boolean { + return isAddress(data.address) && typeof data.token === "string" && data.token.length > 0; +}web/src/utils/atlas/fetchUser.ts (4)
4-4: Use more specific import pathConsider using a more specific import path for OPTIONS to improve maintainability.
-import { OPTIONS } from "utils/wrapWithToast"; +import { OPTIONS } from "@/utils/wrapWithToast";
15-23: Add field validation in GraphQL queryConsider adding field validation to handle potential null values and improve error handling. You can use the
@skipor@includedirectives for conditional field inclusion.const query = gql` query GetUser { user { - email + email @include(if: $includeEmail) isEmailVerified emailUpdateableAt } } `;
31-33: Remove console.log and improve error loggingProduction code should use proper error logging instead of console.log. Consider using a logging service or removing the log statement entirely.
- // eslint-disable-next-line no-console - console.log("Error fetching user :", { errors }); + // TODO: Implement proper error logging
25-47: Enhance error handling and add request timeoutThe current implementation could benefit from:
- More specific error types
- Request timeout handling
- Retry mechanism for transient failures
+const TIMEOUT_MS = 5000; +const MAX_RETRIES = 3; + export function fetchUser(client: GraphQLClient): Promise<User> { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), TIMEOUT_MS); + return toast.promise<User, Error>( client - .request<GetUserResponse>(query) + .request<GetUserResponse>(query, {}, { signal: controller.signal }) .then((response) => response.user) .catch((errors) => { + clearTimeout(timeoutId); const errorMessage = Array.isArray(errors?.response?.errors) ? errors.response.errors[0]?.message + : errors instanceof DOMException && errors.name === "AbortError" + ? "Request timed out" : "Error fetching user"; throw Error(errorMessage); }),web/src/layout/Header/navbar/Menu/Settings/Notifications/index.tsx (1)
Line range hint
38-38: Consider adding explicit type for the children prop.The EnsureAuth wrapper uses a JSX fragment with children. For better type safety, consider explicitly typing the children prop in the ISettings interface.
- <> + <React.Fragment> <HeaderNotifs /> <FormContactDetails toggleIsSettingsOpen={toggleIsSettingsOpen} /> - </> + </React.Fragment>web/src/utils/atlas/addUser.ts (3)
5-5: Use relative import path for local modulesThe import path for local utilities should use a relative path format.
-import { OPTIONS } from "utils/wrapWithToast"; +import { OPTIONS } from "./wrapWithToast";
13-15: Consider adding email validation at type levelThe email field could benefit from runtime type validation to ensure valid email formats before making the GraphQL request.
+import { z } from "zod"; + export type AddUserData = { - email: string; + email: z.string().email(); };
31-33: Remove console.log from production codeProduction code should use proper error tracking/logging services instead of console.log.
- // eslint-disable-next-line no-console - console.log("Add User error:", { errors }); + // Consider using a proper error tracking service + // Example: Sentry.captureException(errors);web/src/utils/atlas/updateEmail.ts (3)
5-5: Consider using absolute imports for better maintainability.The import path
utils/wrapWithToastshould be consistent with other imports. Consider using an absolute import path (e.g.,@/utils/wrapWithToastor similar) based on your project's path aliasing configuration.
7-11: Consider adding email format validation in the GraphQL schema.While client-side validation might exist, adding email format validation at the GraphQL schema level provides an additional layer of security and data integrity. This could prevent invalid email formats from reaching the backend.
39-49: Consider more descriptive toast messages.The current toast messages could be more informative to provide better user feedback.
{ - pending: `Updating Email ...`, - success: "Email Updated successfully!", + pending: `Updating your email address...`, + success: "Your email has been updated successfully! Please check your inbox for verification.", error: { render({ data: error }) { - return `Updating Email failed: ${error?.message}`; + return `Failed to update email: ${error?.message}. Please try again.`; }, }, },web/src/utils/atlas/loginUser.ts (1)
1-4: Consider using absolute imports for better maintainability.The relative import path
utils/wrapWithToastcould be made more explicit using the@/orsrc/prefix to clearly indicate the import source.-import { OPTIONS } from "utils/wrapWithToast"; +import { OPTIONS } from "@/utils/wrapWithToast";web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/FormContact.tsx (1)
Line range hint
1-67: Consider adding input type validation.The component seems to be used for contact information, but there's no explicit input type validation. Consider adding appropriate HTML5 input attributes for better user experience and browser-level validation.
<StyledField variant={fieldVariant} value={contactInput} onChange={handleInputChange} placeholder={contactPlaceholder} + type="email" // or "tel" depending on the contact type + inputMode="email" // or "tel" based on contact type />web/src/layout/Header/navbar/Menu/Settings/index.tsx (2)
72-77: Consider enhancing the click-away handler implementationWhile the basic functionality works, there are several improvements that could make the code more robust and maintainable:
- The hash value "#notifications" should be moved to a constant
- Consider adding error handling for navigation
- Document why replace:true is necessary
Consider applying these changes:
+ const NOTIFICATION_HASH = "#notifications"; + const Settings: React.FC<ISettings> = ({ toggleIsSettingsOpen }) => { const [currentTab, setCurrentTab] = useState<number>(0); const containerRef = useRef(null); const location = useLocation(); const navigate = useNavigate(); useClickAway(containerRef, () => { toggleIsSettingsOpen(); - if (location.hash.includes("#notifications")) navigate("#", { replace: true }); + try { + // Replace history to prevent back button from returning to notifications + if (location.hash.includes(NOTIFICATION_HASH)) { + navigate("#", { replace: true }); + } + } catch (error) { + console.error("Navigation failed:", error); + } });
Line range hint
64-91: Consider architectural improvements for better maintainability and accessibilityThe component could benefit from several architectural improvements:
- Add prop types documentation for better maintainability
- Implement error boundaries for graceful error handling
- Consider memoization for performance optimization
- Enhance accessibility with ARIA attributes
Here's a suggested approach:
interface SettingsProps extends ISettings { toggleIsSettingsOpen: () => void; } const Settings: React.FC<SettingsProps> = React.memo(({ toggleIsSettingsOpen }) => { // ... existing state and refs ... return ( <Container ref={containerRef} role="dialog" aria-label="Settings" > <StyledSettingsText id="settings-title">Settings</StyledSettingsText> <StyledTabs currentValue={currentTab} items={TABS} callback={(n: number) => { setCurrentTab(n); }} aria-labelledby="settings-title" /> {/* ... rest of the component ... */} </Container> ); }); Settings.displayName = 'Settings';web/src/utils/date.ts (1)
43-69: Add input validation and consider edge cases.The function logic is sound, but could benefit from additional error handling.
Consider applying these improvements:
export function timeLeftUntil(isoString: string): string { + if (!isoString) { + throw new Error("Date string is required"); + } + const targetDate = new Date(isoString); + + // Check if date is valid + if (isNaN(targetDate.getTime())) { + throw new Error("Invalid date format"); + } + const now = new Date(); const timeDifference = targetDate.getTime() - now.getTime(); if (timeDifference <= 0) { return "The date has already passed."; } const secondsLeft = Math.floor(timeDifference / 1000); const minutesLeft = Math.floor(secondsLeft / 60); const hoursLeft = Math.floor(minutesLeft / 60); const daysLeft = Math.floor(hoursLeft / 24); + // Handle extremely large time differences (e.g., > 30 days) + if (daysLeft > 30) { + const options: Intl.DateTimeFormatOptions = { + year: "numeric", + month: "long", + day: "numeric", + hour: "2-digit", + minute: "2-digit" + }; + return `on ${targetDate.toLocaleDateString("en-US", options)}`; + } if (secondsLeft < 60) {web/src/app.tsx (1)
33-50: Consider adding error boundaries for better error handling.While the routing structure is correct, consider wrapping key routes with error boundaries to gracefully handle component-level errors, especially for the new Settings functionality.
Example implementation:
import { ErrorBoundary } from 'react-error-boundary'; import ErrorFallback from './components/ErrorFallback'; // Wrap route content: <Route path="settings/*" element={ <ErrorBoundary FallbackComponent={ErrorFallback}> <Settings /> </ErrorBoundary> } />web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/EmailVerificationInfo.tsx (4)
31-31: Enhance accessibility with semantic HTML.The
InfoSubtitlestyled component should use a more semantic HTML element since it contains interactive content.-const InfoSubtitle = styled.label``; +const InfoSubtitle = styled.div` + color: ${({ theme }) => theme.text}; +`;
59-61: Add JSDoc documentation for better prop understanding.Consider adding documentation to explain the purpose and usage of the interface props.
+/** + * Props for EmailVerificationInfo component + * @interface IEmailInfo + * @property {() => void} toggleIsSettingsOpen - Callback to toggle settings panel visibility + */ interface IEmailInfo { - toggleIsSettingsOpen: () => void; + toggleIsSettingsOpen: () => void; }
66-66: Address TODO comment about toast notifications.The TODO comment indicates missing user feedback for email updates.
Would you like me to help implement proper toast notifications for email verification status updates?
82-95: Optimize conditional rendering.The empty fragment fallback can be eliminated by using a guard clause.
- return userExists && !user?.isEmailVerified ? ( + if (!userExists || user?.isEmailVerified) return null; + + return ( <InfoContainer> <StyledHourglassIcon /> <InfoInnerContainer> <InfoTitle>Email Verification Pending</InfoTitle> <InfoSubtitle> We sent you a verification email. Please, verify it. <br /> Didn't receive the email? <StyledButton text="Resend it" onClick={resendEmail} /> </InfoSubtitle> </InfoInnerContainer> </InfoContainer> - ) : ( - <></> );web/src/components/ActionButton/Modal/EvidenceUpload.tsx (1)
File type validation needed for evidence uploads
The review comment is correct. The code assumes PDF files but lacks validation:
- The component uses
FileUploaderfrom@kleros/ui-components-librarybut hardcodesfileTypeExtension: "pdf"without validating the actual file type- Looking at other file upload handlers in the codebase (e.g.,
LogoUpload.tsx), we can see proper type validation being implemented- The suggested fix to add file type validation is appropriate to prevent inconsistencies
The original review's suggested implementation is a good solution:
+ const ALLOWED_EVIDENCE_TYPES = ['application/pdf']; + const handleFileUpload = (file: File) => { setIsEvidenceUploading(true); + if (!ALLOWED_EVIDENCE_TYPES.includes(file.type)) { + toast.error("Only PDF files are allowed for evidence", toastOptions); + setIsEvidenceUploading(false); + return; + } uploadFile(file, Roles.Evidence)🔗 Analysis chain
Line range hint
62-76: Verify file type validationThe component assumes PDF files (hardcoded extension and uploader message) but there's no explicit validation of the uploaded file type. This could lead to inconsistencies if non-PDF files are uploaded.
Let's check if there's file type validation elsewhere in the codebase:
Consider adding file type validation:
+ const ALLOWED_EVIDENCE_TYPES = ['application/pdf']; + const handleFileUpload = (file: File) => { setIsEvidenceUploading(true); + if (!ALLOWED_EVIDENCE_TYPES.includes(file.type)) { + toast.error("Only PDF files are allowed for evidence", toastOptions); + setIsEvidenceUploading(false); + return; + } uploadFile(file, Roles.Evidence)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for file type validation in FileUploader component and related files rg -l "FileUploader|accept=|application\/pdf"Length of output: 183
Script:
#!/bin/bash # Search for file type validation patterns with proper escaping rg -l "FileUploader|accept=|application/pdf" # Search for file type validation logic rg -B2 -A2 "file\.(type|extension)" # Look for file upload related components ast-grep --pattern 'const handleFileUpload = ($_) => { $$$ }'Length of output: 6855
Script:
#!/bin/bash # Let's check the file input component in EvidenceUpload for any accept attribute rg -A 5 -B 5 "<input.*type=.file" web/src/components/ActionButton/Modal/EvidenceUpload.tsx # Also check if there's a FileUploader component being used rg -A 5 -B 5 "<FileUploader" web/src/components/ActionButton/Modal/EvidenceUpload.tsxLength of output: 180
Script:
#!/bin/bash # Let's check the entire EvidenceUpload component content cat web/src/components/ActionButton/Modal/EvidenceUpload.tsx # Also check for any shared file upload components fd FileUploaderLength of output: 3155
Script:
#!/bin/bash # Let's check the FileUploader component from ui-components-library for any built-in validation rg -A 10 -B 10 "FileUploader.*=.*{" node_modules/@kleros/ui-components-library/ # Check for any props or types related to FileUploader rg -A 5 "interface.*FileUploader" node_modules/@kleros/ui-components-library/Length of output: 345
web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx (4)
1-19: Consider reorganizing imports for better maintainability.The imports could be better organized by grouping them into:
- External dependencies
- Internal components/contexts
- Utils/constants
- Types
- Styles
Consider updating the import path for
isUndefinedto be consistent with other utility imports:-import { isUndefined } from "src/utils"; +import { isUndefined } from "utils";
56-58: Consider improving email update validation logic.The email update validation could be more robust and reusable:
-const isEmailUpdateable = user?.email - ? !isUndefined(user?.emailUpdateableAt) && new Date(user.emailUpdateableAt).getTime() < new Date().getTime() - : true; +const isEmailUpdateable = !user?.email || + (user.emailUpdateableAt && new Date(user.emailUpdateableAt).getTime() < Date.now());
Line range hint
102-113: Remove commented code.Remove the commented out Telegram-related code to maintain cleaner codebase. If needed, this code can be retrieved from version control history.
129-134: Consider simplifying button disabled logic.The current disabled condition is complex and could be simplified for better readability.
-disabled={ - !isEditingEmail || !emailIsValid || isAddingUser || isFetchingUser || isUpdatingUser || !isEmailUpdateable -} +const isLoading = isAddingUser || isFetchingUser || isUpdatingUser; +const isValidSubmission = isEditingEmail && emailIsValid && isEmailUpdateable; +disabled={isLoading || !isValidSubmission}web/src/pages/Settings/EmailConfirmation/index.tsx (4)
72-92: Consider consolidating icon container components and improving type safety.The
IconContainerandHeaderIconContainercomponents share similar SVG styling patterns. Consider:
- Creating a base icon container component
- Adding proper TypeScript types for theme colors
+type ThemeColor = keyof typeof theme; + +const BaseIconContainer = styled.div<{ size: string; color: ThemeColor }>` + svg { + width: ${({ size }) => size}; + height: ${({ size }) => size}; + path { + fill: ${({ theme, color }) => theme[color]}; + } + } +`; + -const HeaderIconContainer = styled.div<{ iconColor: string }>` +const HeaderIconContainer = styled(BaseIconContainer).attrs({ + size: "64px" +})<{ iconColor: ThemeColor }>` - svg { - width: 64px; - height: 64px; - ${({ iconColor }) => css` - path { - fill: ${({ theme }) => theme[iconColor]}; - } - `} - } `; -const IconContainer = styled.div` +const IconContainer = styled(BaseIconContainer).attrs({ + size: "250px", + color: "whiteBackground" +})` - svg { - width: 250px; - height: 250px; - path { - fill: ${({ theme }) => theme.whiteBackground}; - } - } `;
94-129: Add TypeScript types and prepare for internationalization.The message configurations would benefit from proper TypeScript types and preparation for future internationalization.
+type MessageConfig = { + headerMsg: string; + subtitleMsg: string; + buttonMsg: string; + buttonTo: string; + Icon: React.FC<React.SVGProps<SVGSVGElement>>; + color: ThemeColor; +}; + +// Consider moving these to a translations file +const MESSAGES = { + EMAIL_VERIFICATION: { + INVALID_LINK: "Invalid Link!", + INVALID_SUBTITLE: "Oops, seems like you followed an invalid link.", + // ... other messages + } +} as const; + -const messageConfigs = { +const messageConfigs: Record<string, MessageConfig> = { invalid: { - headerMsg: "Invalid Link!", - subtitleMsg: "Oops, seems like you followed an invalid link.", + headerMsg: MESSAGES.EMAIL_VERIFICATION.INVALID_LINK, + subtitleMsg: MESSAGES.EMAIL_VERIFICATION.INVALID_SUBTITLE, // ... rest of the config }, // ... other configs };
158-163: Optimize message configuration selection.The useMemo implementation could be more explicit about its conditions and use early returns for better readability.
const { headerMsg, subtitleMsg, buttonMsg, buttonTo, Icon, color } = useMemo(() => { - if (!address || !isAddress(address) || !token || isTokenInvalid) return messageConfigs.invalid; - if (isError) return messageConfigs.error; - if (isConfirmed) return messageConfigs.confirmed; - return messageConfigs.expired; + if (!isValidEmailConfirmation(address, token) || isTokenInvalid) { + return messageConfigs.invalid; + } + + if (isError) { + return messageConfigs.error; + } + + if (isConfirmed) { + return messageConfigs.confirmed; + } + + return messageConfigs.expired; }, [address, token, isError, isConfirmed, isTokenInvalid]);
165-185: Consider extracting route constants and improving accessibility.The navigation paths are hardcoded and the component could benefit from better accessibility.
+const ROUTES = { + HOME: "/", + SETTINGS_NOTIFICATIONS: "/#notifications", +} as const; return ( - <Container> + <Container role="main" aria-live="polite"> {isConfirming ? ( - <Loader width={"148px"} height={"148px"} /> + <Loader width="148px" height="148px" aria-label="Confirming email" /> ) : ( <> <InfoWrapper> <HeaderIconContainer iconColor={color}> - <Icon /> + <Icon aria-hidden="true" /> </HeaderIconContainer> <Header fontColor={color}>{headerMsg}</Header> <Subtitle>{subtitleMsg}</Subtitle> - <Button text={buttonMsg} onClick={() => navigate(buttonTo)} /> + <Button + text={buttonMsg} + onClick={() => navigate(ROUTES[buttonTo] ?? ROUTES.HOME)} + aria-label={buttonMsg} + /> </InfoWrapper> <IconContainer> - <Icon /> + <Icon aria-hidden="true" /> </IconContainer> </> )} </Container> );web/src/utils/atlas/uploadToIpfs.ts (3)
5-7: Consider simplifying theProductsenumCurrently, the
Productsenum contains only one value,Curate. If there's no immediate plan to add more products, you might simplify the code by using a string literal type instead of an enum.-export enum Products { - Curate = "Curate", -} +export type Products = "Curate";
47-48: Enhance error handling when parsing the error response failsIf
response.json()fails (e.g., due to a non-JSON response), the current implementation defaults to a generic error message. Consider including theresponse.statusTextor the raw response body to provide more informative error messages.-const error = await response.json().catch(() => ({ message: "Error uploading to IPFS" })); +let errorMessage = "Error uploading to IPFS"; +try { + const errorResponse = await response.json(); + errorMessage = errorResponse.message || errorMessage; +} catch { + errorMessage = response.statusText || errorMessage; +}Then, update the error handling:
-if (response.status === 401) throw new AuthorizationError(error.message); -throw new Error(error.message); +if (response.status === 401) throw new AuthorizationError(errorMessage); +throw new Error(errorMessage);
68-77: Simplify theAuthorizationErrorclass definitionThe custom
AuthorizationErrorclass manually sets thenameproperty and captures the stack trace. Since extending the built-inErrorclass already handles thenameproperty and stack trace, you can simplify the class definition.-export class AuthorizationError extends Error { - readonly name = "AuthorizationError" as const; - constructor(message: string) { - super(message); - - if (Error.captureStackTrace) { - Error.captureStackTrace(this, this.constructor); - } - } -} +export class AuthorizationError extends Error { + constructor(message: string) { + super(message); + this.name = "AuthorizationError"; + } +}web/src/context/AtlasProvider.tsx (2)
189-189: Useconsole.errorInstead ofconsole.logfor Error LoggingErrors are currently logged using
console.login multiple catch blocks. Usingconsole.erroris more appropriate for logging errors, as it differentiates error messages from standard logs and improves visibility in debugging tools.Apply this diff to standardize error logging:
// Replace console.log with console.error in error handling - console.log("Authorize User Error : ", err?.message); + console.error("Authorize User Error : ", err?.message); - console.log("Add User Error : ", err?.message); + console.error("Add User Error : ", err?.message); - console.log("Update User Email Error : ", err?.message); + console.error("Update User Email Error : ", err?.message); - console.log("Upload File Error : ", err?.message); + console.error("Upload File Error : ", err?.message); - console.log("Confirm Email Error : ", err?.message); + console.error("Confirm Email Error : ", err?.message);Also applies to: 212-212, 238-238, 266-266, 290-290
298-334: Re-evaluate the Use ofuseMemofor Context ValueThe dependency array for
useMemoincludes many state variables that frequently change, which may negate the performance benefits as the memoized value will recompute often. Consider removinguseMemoor grouping related state variables to optimize performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
web/src/assets/svgs/icons/minus-circle.svgis excluded by!**/*.svgweb/src/assets/svgs/icons/warning-outline.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (44)
web/netlify/functions/authUser.ts(0 hunks)web/netlify/functions/fetch-settings.ts(0 hunks)web/netlify/functions/getNonce.ts(0 hunks)web/netlify/functions/update-settings.ts(0 hunks)web/netlify/functions/uploadToIPFS.ts(0 hunks)web/netlify/middleware/authMiddleware.ts(0 hunks)web/package.json(1 hunks)web/scripts/generateSupabaseTypes.sh(0 hunks)web/src/app.tsx(1 hunks)web/src/components/ActionButton/Modal/Buttons/index.tsx(1 hunks)web/src/components/ActionButton/Modal/EvidenceUpload.tsx(3 hunks)web/src/components/EnsureAuth.tsx(1 hunks)web/src/consts/index.ts(1 hunks)web/src/context/AtlasProvider.tsx(1 hunks)web/src/hooks/queries/useUserSettings.tsx(0 hunks)web/src/hooks/useSessionStorage.ts(1 hunks)web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/EmailVerificationInfo.tsx(1 hunks)web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/FormContact.tsx(1 hunks)web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx(4 hunks)web/src/layout/Header/navbar/Menu/Settings/Notifications/index.tsx(1 hunks)web/src/layout/Header/navbar/Menu/Settings/index.tsx(2 hunks)web/src/pages/Settings/EmailConfirmation/index.tsx(1 hunks)web/src/pages/Settings/index.tsx(1 hunks)web/src/pages/SubmitItem/ItemField/FieldInput/FileInput.tsx(2 hunks)web/src/pages/SubmitItem/ItemField/FieldInput/ImageInput.tsx(2 hunks)web/src/pages/SubmitItem/index.tsx(1 hunks)web/src/pages/SubmitList/ListParameters/LogoUpload.tsx(3 hunks)web/src/pages/SubmitList/ListParameters/Policy.tsx(2 hunks)web/src/pages/SubmitList/index.tsx(1 hunks)web/src/types/supabase-datalake.ts(0 hunks)web/src/types/supabase-notification.ts(0 hunks)web/src/utils/atlas/addUser.ts(1 hunks)web/src/utils/atlas/confirmEmail.ts(1 hunks)web/src/utils/atlas/createMessage.ts(1 hunks)web/src/utils/atlas/fetchUser.ts(1 hunks)web/src/utils/atlas/getNonce.ts(1 hunks)web/src/utils/atlas/index.ts(1 hunks)web/src/utils/atlas/loginUser.ts(1 hunks)web/src/utils/atlas/updateEmail.ts(1 hunks)web/src/utils/atlas/uploadToIpfs.ts(1 hunks)web/src/utils/date.ts(1 hunks)web/src/utils/index.ts(1 hunks)web/src/utils/uploadFileToIPFS.ts(0 hunks)web/src/utils/uploadSettingsToSupabase.ts(0 hunks)
💤 Files with no reviewable changes (12)
- web/netlify/functions/authUser.ts
- web/netlify/functions/fetch-settings.ts
- web/netlify/functions/getNonce.ts
- web/netlify/functions/update-settings.ts
- web/netlify/functions/uploadToIPFS.ts
- web/netlify/middleware/authMiddleware.ts
- web/scripts/generateSupabaseTypes.sh
- web/src/hooks/queries/useUserSettings.tsx
- web/src/types/supabase-datalake.ts
- web/src/types/supabase-notification.ts
- web/src/utils/uploadFileToIPFS.ts
- web/src/utils/uploadSettingsToSupabase.ts
✅ Files skipped from review due to trivial changes (2)
- web/src/pages/SubmitList/index.tsx
- web/src/utils/atlas/index.ts
🔇 Additional comments (42)
web/src/hooks/useSessionStorage.ts (1)
6-7: LGTM! Verify session persistence requirements.
The switch from localStorage to sessionStorage is a good security improvement as it ensures data is cleared when the browser session ends. However, this means users will need to re-authenticate after closing their browser.
Let's verify this is the intended behavior and that we're not missing any localStorage dependencies:
✅ Verification successful
Both localStorage and sessionStorage are being used intentionally for different purposes
The codebase shows a clear separation of concerns:
useSessionStorageis specifically used for auth token storage inAtlasProvider.tsx, which is the correct choice for security-sensitive datauseLocalStorageis intentionally kept for user preferences and form data persistence in:- Theme preferences
- List view preferences
- Form fields and submission data
- Registry item IDs
The switch to sessionStorage for the auth token is appropriate and doesn't conflict with other storage needs. The implementation maintains proper separation between security-sensitive data (session) and user preferences (local).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining localStorage usage that might need to be updated
rg -g '!*.lock' -g '!*.md' 'localStorage'
# Search for any direct sessionStorage usage to ensure consistency
rg -g '!*.lock' -g '!*.md' 'sessionStorage'
Length of output: 481
Script:
#!/bin/bash
# Let's check where these hooks are being used
rg -l "useLocalStorage|useSessionStorage" --type ts --type tsx
# And get the actual usage context
rg "useLocalStorage|useSessionStorage" -A 3 -B 3 --type ts --type tsx
Length of output: 192
Script:
#!/bin/bash
# Let's try without file type restrictions to find usages
rg -l "useLocalStorage|useSessionStorage"
# And get the actual usage context
rg "useLocalStorage|useSessionStorage" -A 3 -B 3
Length of output: 8194
web/src/utils/atlas/createMessage.ts (2)
1-3: LGTM! Clean and specific imports.
The imports are well-organized and specific, which is good for tree-shaking.
5-23: Verify nonce generation and usage.
The function accepts a nonce parameter, but we should verify how this nonce is generated and managed to ensure security.
✅ Verification successful
Let me gather more information about the getNonce function since it appears to be the source of nonce generation.
Let me check for the GraphQL query definition and the Netlify function that handles nonce generation.
Based on the gathered information, I can now provide the final response:
Nonce generation and usage is properly implemented
The nonce is securely managed through a server-side GraphQL mutation that generates a unique nonce per address. The implementation shows:
- Nonce is generated server-side through a GraphQL mutation
- It's tied to a specific Ethereum address
- The nonce is used immediately in a SIWE (Sign-In with Ethereum) message with a 10-minute expiration
- The signed message is verified server-side during login
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the nonce is generated and used across the codebase
# Search for nonce generation patterns
echo "Checking nonce generation patterns..."
rg -A 5 "generateNonce|createNonce|nonce.*=" --type ts
# Search for createMessage usage to understand nonce handling
echo "Checking createMessage usage..."
rg -A 5 "createMessage\(" --type ts
Length of output: 2500
Script:
#!/bin/bash
# Search for getNonce implementation and its related code
echo "Checking getNonce implementation..."
rg -A 10 "getNonce|async function getNonce" --type ts
# Search for any GraphQL operations related to nonce
echo "Checking GraphQL operations for nonce..."
rg -A 5 "query.*Nonce|mutation.*Nonce" --type graphql --type ts
Length of output: 3709
Script:
#!/bin/bash
# Search for GraphQL query definition for nonce
echo "Checking for GraphQL query definitions..."
rg -A 5 "GetNonce|gql.*nonce" --type ts
# Search for Netlify function implementation
echo "Checking Netlify function implementation..."
rg -l "getNonce" netlify/functions/
Length of output: 1401
web/src/components/EnsureAuth.tsx (2)
1-12: LGTM! Clean and focused imports.
The imports have been effectively streamlined to include only the essential dependencies needed for the new authentication flow.
30-30: Verify all imports are updated for default export.
The export has been changed from named to default. Let's verify all imports have been updated accordingly.
✅ Verification successful
All imports are correctly using default import syntax
The verification shows that all imports of EnsureAuth are using the correct default import syntax (import EnsureAuth from "components/EnsureAuth"). No named imports were found, confirming that the codebase is consistent with the export change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining named imports of EnsureAuth that need to be updated
# Test: Search for named imports. Expect: No results
rg "import \{ .*EnsureAuth.* \} from ['\"]components/EnsureAuth['\"]"
# Test: Verify default imports are used. Expect: Updated import statements
rg "import EnsureAuth from ['\"]components/EnsureAuth['\"]"
Length of output: 526
web/src/pages/Settings/index.tsx (2)
1-8: LGTM! Clean and well-organized imports.
The imports are properly organized and all are being utilized in the component.
21-23: Verify route configuration in the main app router.
Let's ensure this settings route is properly configured in the main application router.
✅ Verification successful
Route configuration is properly set up in the main app router
The route is correctly configured in web/src/app.tsx using <Route path="settings/*" element={<Settings />} />. The /* wildcard pattern allows for nested routes like the email-confirmation route you're adding.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the settings route is properly configured in the app router
# Look for the settings route configuration
rg -l "settings" src/app.tsx src/router.tsx src/routes.tsx 2>/dev/null
# Check the route configuration pattern
ast-grep --pattern 'Route $_ element={<Settings />} path="settings/*"'
Length of output: 131
Script:
#!/bin/bash
# Let's try a broader search for route configurations and settings-related components
# Search for Settings component imports and route configurations
rg -A 3 "Settings|Route.*settings" --type tsx --type ts
# Search for any file containing "settings" in its path
fd -e tsx -e ts -e js "settings" -t f
# Look for route configurations with a more general pattern
ast-grep --pattern 'Route path="$_" element={$_}'
Length of output: 175
Script:
#!/bin/bash
# Let's try with correct file extensions and simpler patterns
# Find all TypeScript/JavaScript files containing "settings"
fd -e ts -e tsx -e js -e jsx "settings"
# Search for route configurations in all TypeScript/JavaScript files
rg "Route.*path.*settings" -t js -t ts
# Look for the Settings component definition
rg "export.*Settings" -t js -t ts
Length of output: 515
web/src/utils/atlas/confirmEmail.ts (1)
1-11: Verify the Address scalar type implementation.
The GraphQL mutation looks well-structured. However, let's verify the implementation of the custom Address scalar type to ensure proper validation.
✅ Verification successful
Based on the search results, I can now provide a final response:
The Address type in the mutation is correctly implemented and validated.
The codebase shows that:
- The
Addresstype is imported fromviem, a widely-used Ethereum utility library - Address validation is properly handled using
isAddress()throughout the codebase - The email confirmation functionality uses proper address validation before processing
- The mutation's
addressparameter is used consistently with the rest of the codebase's address handling
The implementation is secure and follows best practices for Ethereum address handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for Address scalar type definition
# Expected: GraphQL schema file with Address scalar definition
# Search for Address scalar type definition
rg -t graphql "scalar Address"
# Search for Address type usage in other mutations
ast-grep --pattern 'mutation $_($address: Address'
Length of output: 117
Script:
#!/bin/bash
# Let's try different approaches to find the Address type definition
# Search for Address type in all files
rg "type Address" -A 3
# Search for Address in schema files with common extensions
rg "Address" -g "*.graphql" -g "*.gql" -g "*.schema*"
# Search for files that might contain GraphQL schema
fd "schema" --type f
# Look for Address type imports
rg "import.*Address.*from"
Length of output: 259525
Script:
#!/bin/bash
# Let's search for the actual implementation and usage of Address type/scalar
# Search for GraphQL schema files with common extensions
fd "schema.*" --type f
# Search for Address type definition in schema files
rg -t graphql "scalar Address|type Address" --no-ignore
# Search for Address type imports and usage in TypeScript/JavaScript files
rg "import.*Address.*from|type Address =|interface Address" --type ts --type js
# Search for Address validation or handling
rg "isAddress|validateAddress|Address\." --type ts --type js
Length of output: 4840
web/src/pages/SubmitItem/ItemField/FieldInput/FileInput.tsx (2)
7-8: LGTM! Clean import of new dependencies.
The new imports for AtlasProvider and Roles are correctly added and align with the PR's objective of integrating Atlas functionality.
23-30:
Enhance error handling and user feedback.
While the implementation successfully integrates with AtlasProvider, there are some concerns regarding error handling and user feedback:
- Upload failures are only logged to console, making it difficult for users to understand what went wrong
- The previous file type validation has been removed
Consider implementing this improved version:
const { uploadFile } = useAtlasProvider();
const handleFileUpload = (file: File) => {
+ setIsUploading(true);
uploadFile(file, Roles.CurateItemFile)
.then(async (fileURI) => {
- if (!fileURI) return;
+ if (!fileURI) {
+ toast.error("File upload failed. Please try again.");
+ return;
+ }
handleWrite(fileURI);
+ toast.success("File uploaded successfully!");
})
- .catch((err) => console.log(err))
+ .catch((err) => {
+ console.error(err);
+ toast.error("Failed to upload file. Please try again.");
+ })
- .finally();
+ .finally(() => setIsUploading(false));
};Let's verify if AtlasProvider handles file type validation:
web/src/pages/SubmitItem/ItemField/FieldInput/ImageInput.tsx (1)
7-8: LGTM: Clean import changes for Atlas integration.
The new imports align well with the PR's objective of migrating from serverless functions to Atlas backend.
web/src/utils/atlas/getNonce.ts (1)
1-8: LGTM! Clean imports and type definitions.
The imports are well-organized and the type definition is clear and focused.
web/src/utils/atlas/fetchUser.ts (1)
1-47: Verify integration with AtlasProvider
Let's verify how this utility is integrated with the AtlasProvider context.
✅ Verification successful
Let me check the actual implementation in AtlasProvider to verify the integration.
Let me try one more search with correct file extensions to get a complete picture of fetchUser usage.
Based on the previous results, I can now generate the final response as we have enough context about the integration:
✅ fetchUser integration with AtlasProvider is properly implemented
The utility is well-integrated within AtlasProvider:
- Used within a React Query setup with proper error handling
- Called conditionally based on verification status
- Properly refreshed on address changes and verification status updates
- Error states are handled at both utility and provider levels
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for fetchUser usage in AtlasProvider
rg -l "fetchUser" "web/src/context"
# Check for proper error handling in the provider
ast-grep --pattern 'try {
$$$
fetchUser($$$)
$$$
} catch (error) {
$$$
}'
Length of output: 151
Script:
#!/bin/bash
# Get the implementation details from AtlasProvider
rg -A 10 "fetchUser" "web/src/context/AtlasProvider.tsx"
# Also check if there are any other components using fetchUser directly
rg "fetchUser" --type typescript --type tsx
Length of output: 2551
Script:
#!/bin/bash
# Search for all files using fetchUser
rg "fetchUser" --type ts --type tsx
# Also check the error handling in AtlasProvider
rg "error|catch" "web/src/context/AtlasProvider.tsx"
Length of output: 537
web/src/layout/Header/navbar/Menu/Settings/Notifications/index.tsx (1)
7-7: LGTM! Verifying the default export.
The change from named to default import aligns with the PR's refactoring objectives.
Let's verify that EnsureAuth actually exports a default export:
✅ Verification successful
Import change verified and correct
The EnsureAuth component at web/src/components/EnsureAuth.tsx correctly uses a default export, confirming that the import change in web/src/layout/Header/navbar/Menu/Settings/Notifications/index.tsx is valid and properly aligned with the component's export pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify EnsureAuth default export exists
# Expected: Should find default export in EnsureAuth component
ast-grep --pattern 'export default $_' components/EnsureAuth
Length of output: 131
Script:
#!/bin/bash
# First, let's find the actual path of EnsureAuth component
fd -t f "EnsureAuth" --exec echo '{}'
# Then, let's check for default exports in those files
echo "=== Checking for default exports ==="
fd -t f "EnsureAuth" --exec ast-grep --pattern 'export default $_' {}
# Also check for named exports to see the current export pattern
echo "=== Checking for named exports ==="
fd -t f "EnsureAuth" --exec ast-grep --pattern 'export { $_ }' {}
Length of output: 439
web/src/utils/atlas/addUser.ts (1)
7-11: Consider adding input validation constraints to GraphQL mutation
The mutation accepts user settings without explicit validation constraints. Consider adding validation directives or input constraints in the GraphQL schema to ensure data integrity.
web/src/utils/atlas/updateEmail.ts (2)
13-19: LGTM! Well-structured type definitions.
The types are clear, focused, and properly separated between input and response interfaces.
1-50: Verify integration with AtlasProvider and email verification flow.
Let's verify that this function is properly integrated with the email verification flow mentioned in the PR objectives.
✅ Verification successful
Integration with AtlasProvider and email verification flow is properly implemented
The verification shows that:
- The
updateEmailfunction is correctly integrated through theuseAtlasProviderhook in both components - It's used in two appropriate contexts:
- In FormContactDetails for updating the user's email
- In EmailVerificationInfo for resending verification emails
- The implementation includes proper email verification state handling and UI feedback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how this function is used in the email verification flow
echo "Checking email verification integration..."
rg -l "updateEmail.*AtlasProvider" web/src/
rg -l "EmailVerificationInfo" web/src/
# Check for any duplicate email update implementations
echo "Checking for duplicate implementations..."
rg -l "mutation.*updateEmail" web/src/
Length of output: 689
Script:
#!/bin/bash
# Let's check the implementation details of the components using updateEmail
echo "Checking FormContactDetails implementation..."
rg -A 10 "updateEmail" web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx
echo "\nChecking EmailVerificationInfo implementation..."
rg -A 10 "updateEmail" web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/EmailVerificationInfo.tsx
# Check if there are any other email update mutations in the codebase
echo "\nChecking for other email update mutations..."
ast-grep --pattern 'mutation $_($$$) {
updateEmail($$$)
}'
Length of output: 2144
web/src/consts/index.ts (1)
16-17: Consider alternative email validation approaches
While regex can provide basic format validation, it's not sufficient for determining if an email is actually valid. Since this PR introduces Atlas integration and email confirmation features, consider using a multi-layered approach:
- Basic format validation (simple regex)
- Domain validation (MX record check)
- Email verification (confirmation link)
Let's verify if email verification is already implemented:
web/src/utils/atlas/loginUser.ts (2)
12-21: LGTM! Well-defined types with proper constraints.
The types are well-structured, and the use of template literal types for the signature format is a good practice for type safety.
23-53: Verify secure token handling in the authentication flow.
Please ensure:
- The message being signed contains a nonce/timestamp to prevent replay attacks
- The access token is stored securely
- The signature verification on the backend validates the correct address
Let's check for proper nonce implementation:
web/src/components/ActionButton/Modal/Buttons/index.tsx (2)
Line range hint 36-49: Verify integration with AtlasProvider.
Since this PR introduces AtlasProvider for user management, please ensure that:
- EnsureAuth properly integrates with AtlasProvider for authentication states
- Error handling is consistent with the new Atlas integration
Let's check the EnsureAuth implementation:
#!/bin/bash
# Description: Verify EnsureAuth implementation and its integration with AtlasProvider
# Expected: EnsureAuth should use AtlasProvider context
# Check EnsureAuth implementation
ast-grep --pattern 'export default function EnsureAuth($$$) { $$$ }'
# Look for AtlasProvider context usage
rg -A 5 "useAtlas|AtlasContext" "components/EnsureAuth"5-5: LGTM! Import statement updated correctly.
The change from named import to default import aligns with the broader refactoring effort.
Let's verify the consistency of EnsureAuth imports across the codebase:
✅ Verification successful
Import statement is correctly standardized across the codebase
The verification confirms that:
- EnsureAuth is correctly exported as a default export in
web/src/components/EnsureAuth.tsx - All imports of EnsureAuth consistently use the default import pattern
import EnsureAuth from "components/EnsureAuth" - No named imports of EnsureAuth remain in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining named imports of EnsureAuth
# Expected: No instances of named imports should remain
# Search for any remaining named imports
rg "import \{ .*EnsureAuth.* \} from ['\"]components/EnsureAuth['\"]"
# Search for the new default import pattern to confirm consistency
rg "import EnsureAuth from ['\"]components/EnsureAuth['\"]"
Length of output: 283
Script:
#!/bin/bash
# Let's try a more comprehensive search approach
# First, let's find the actual location of EnsureAuth component
fd -t f "EnsureAuth"
# Then search for any imports of EnsureAuth with proper path handling
rg "import.*EnsureAuth.*from"
# Also check for exports in potential EnsureAuth files
fd -t f "EnsureAuth" --exec cat {}
Length of output: 1199
web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/FormContact.tsx (1)
41-41:
Breaking Change: Empty inputs are no longer considered valid.
The validation logic has been simplified but introduces a breaking change. Empty inputs that were previously considered valid will now be marked as invalid. This might affect form submission behavior and user experience.
Consider one of these approaches to handle empty inputs consistently:
- Restore the previous behavior for empty inputs
- Update the
fieldVariantlogic to show error state for empty required fields
Let's verify the impact of this change:
Suggested fix to restore previous behavior:
- setContactIsValid(validator.test(contactInput));
+ setContactIsValid(contactInput === "" || validator.test(contactInput));Alternative fix to enforce validation:
const fieldVariant = useMemo(() => {
- if (contactInput === "" || !isEditing) {
+ if (!isEditing) {
return undefined;
}
+ if (contactInput === "") {
+ return "error";
+ }
return contactIsValid ? "success" : "error";
}, [contactInput, contactIsValid, isEditing]);web/src/layout/Header/navbar/Menu/Settings/index.tsx (2)
9-9: LGTM: Clean routing hook imports
The addition of routing hooks is appropriate for the enhanced navigation requirements.
Line range hint 64-91: Verify Atlas provider integration requirements
Given that this PR introduces Atlas integration, we should verify if this component needs to interact with the Atlas provider for settings management.
✅ Verification successful
Atlas provider integration is correctly implemented in child components
The verification shows that Atlas integration is properly handled in the child components:
FormContactDetails/index.tsxusesuseAtlasProviderfor user managementEmailVerificationInfo.tsxusesuseAtlasProviderfor email verification- The parent
Settingscomponent correctly renders these Atlas-integrated components through theNotificationSettingstab
The current implementation follows a proper separation of concerns where Atlas-related functionality is handled by the specific components that need it, rather than the container component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other components use AtlasProvider for settings
echo "Checking for Atlas provider usage in settings-related files..."
rg -l "useAtlas|AtlasProvider" "web/src/layout/Header/navbar/Menu/Settings/"
# Check if General or NotificationSettings components use Atlas
echo "Checking imported components for Atlas integration..."
ast-grep --pattern 'import { useAtlas } from' web/src/layout/Header/navbar/Menu/Settings/General.tsx web/src/layout/Header/navbar/Menu/Settings/Notifications.tsx
Length of output: 788
Script:
#!/bin/bash
# Let's check the actual file structure and content
echo "Checking file structure..."
fd . "web/src/layout/Header/navbar/Menu/Settings/"
echo -e "\nChecking Atlas usage in FormContactDetails..."
rg -C 3 "useAtlas|AtlasProvider" "web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx"
rg -C 3 "useAtlas|AtlasProvider" "web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/EmailVerificationInfo.tsx"
echo -e "\nChecking NotificationSettings component..."
fd "NotificationSettings" "web/src/layout/Header/navbar/Menu/Settings/"
Length of output: 2211
web/src/pages/SubmitList/ListParameters/Policy.tsx (2)
11-12: LGTM: New imports align with Atlas integration.
The addition of useAtlasProvider and Roles imports supports the migration to Atlas-based file handling.
42-42:
Enhance error handling and add file validations.
While the integration with Atlas is good, there are several areas that need improvement:
- Users receive no feedback on upload success/failure
- Missing file type validation could lead to security issues
- No file size limits could impact performance
Consider implementing these improvements:
const { uploadFile } = useAtlasProvider();
+const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5MB
+const ALLOWED_TYPES = ['application/pdf'];
const handleFileUpload = (file: File) => {
+ if (!ALLOWED_TYPES.includes(file.type)) {
+ alert('Only PDF files are allowed');
+ return;
+ }
+
+ if (file.size > MAX_FILE_SIZE) {
+ alert('File size must be less than 5MB');
+ return;
+ }
setIsPolicyUploading(true);
uploadFile(file, Roles.Policy)
.then(async (policyURI) => {
if (!policyURI) return;
setListMetadata({ ...listMetadata, policyURI });
+ alert('Policy file uploaded successfully');
})
- .catch((err) => console.log(err))
+ .catch((err) => {
+ console.error(err);
+ alert('Failed to upload policy file. Please try again.');
+ })
.finally(() => setIsPolicyUploading(false));
};Let's verify if there are any existing file size or type validations in the Atlas provider:
Also applies to: 46-51
✅ Verification successful
Based on the results, I can see that the uploadFile implementation in AtlasProvider doesn't include any file validation. Let me check if there are any validations in the IPFS upload function:
Based on the investigation, I can now provide the final response:
File validation and error handling improvements needed
The review comment is correct. Neither the AtlasProvider nor the IPFS upload implementation includes any file type or size validations. The current implementation also uses toast for IPFS upload feedback but lacks user-friendly error messages for validation failures.
web/src/context/AtlasProvider.tsx: Only logs errors to consoleweb/src/utils/atlas/uploadToIpfs.ts: Uses toast for IPFS feedback but no pre-upload validationsweb/src/pages/SubmitList/ListParameters/Policy.tsx: Needs the suggested validations
The proposed changes to add file type/size validations and improve error handling should be implemented as suggested in the review.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for file validation logic in Atlas-related files
rg -l "MAX_FILE_SIZE|ALLOWED_TYPES" $(fd -t f -e ts -e tsx atlas)
Length of output: 101
Script:
#!/bin/bash
# Let's check the actual implementation of uploadFile in AtlasProvider
rg -A 10 "uploadFile.*=.*" web/src/context/AtlasProvider.tsx
# Also check if there are any file validations in the codebase
rg -A 5 "file\.(size|type)" web/src/pages
Length of output: 902
Script:
#!/bin/bash
# Check for any file validation in the IPFS upload implementation
rg -A 10 "uploadToIpfs" --type ts
# Also check for any existing file validation patterns in the entire codebase
rg -A 5 "(MAX_FILE_SIZE|ALLOWED_TYPES|validateFile|file validation)" --type ts --type tsx
Length of output: 2397
web/src/pages/SubmitList/ListParameters/LogoUpload.tsx (2)
12-13: LGTM: Import changes align with Atlas integration.
The new imports correctly support the migration from direct IPFS upload to Atlas-based file management.
57-59: Verify Atlas role usage across the codebase.
Let's ensure consistent usage of the Logo role for file uploads across the application.
✅ Verification successful
Atlas role usage is correctly implemented
The verification shows that:
Roles.Logois properly defined in theRolesenum inweb/src/utils/atlas/uploadToIpfs.ts- File uploads using Atlas are consistently implemented across the codebase:
- Logo uploads use
Roles.Logo - Evidence uploads use
Roles.Evidence - Item images use
Roles.CurateItemImage - Item files use
Roles.CurateItemFile - Policy uploads use
Roles.Policy
- Logo uploads use
All file uploads are handled through the centralized uploadFile function in AtlasProvider, ensuring consistent behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of file uploads using Atlas
rg "uploadFile.*Roles\.Logo" --type ts
rg "Roles\.Logo" --type ts
# Search for any remaining IPFS upload references that might need migration
rg "uploadFileToIPFS" --type ts
Length of output: 285
Script:
#!/bin/bash
# Let's first check the Roles enum definition
ast-grep --pattern 'enum Roles {
$$$
}'
# Check for any file upload related functions and their role usage
ast-grep --pattern 'function uploadFile($$$) {
$$$
}'
# Check for other file upload implementations
rg "upload.*file" -i --type ts -A 3
Length of output: 10676
web/src/utils/date.ts (2)
34-42: Well-documented function with clear examples!
The JSDoc documentation is comprehensive and includes parameter type, return value, and usage example.
43-69: Verify the usage contexts of this function.
Since this PR involves Atlas integration and user settings management, ensure this function meets all time-formatting requirements across different features (e.g., email confirmation expiry, session timeouts).
Let's check where this function is being used:
web/src/app.tsx (3)
20-21: LGTM: New imports are properly structured.
The imports for AtlasProvider and Settings components are correctly placed with other component imports.
47-47: Verify Settings route configuration and nested routes.
The Settings route is configured with a wildcard path (settings/*), which aligns with the PR objectives mentioning nested routes for email confirmation. However, we should verify the route structure matches the component's implementation.
Let's check the Settings component structure:
#!/bin/bash
# Description: Verify Settings component's route structure
# Check for route definitions in Settings component
echo "Checking Settings component's route structure..."
rg -A 5 "Route" "web/src/pages/Settings"
# Look for potential route paths in Settings
echo "Checking for potential route paths..."
rg -l "useLocation|useNavigate|useParams" "web/src/pages/Settings"29-54: Verify the provider ordering is correct for state dependencies.
The AtlasProvider has been placed at a high level in the provider hierarchy, which is good for global state management. However, we should verify that this ordering doesn't create any dependency issues with other providers.
Let's check for any potential provider dependencies:
✅ Verification successful
Based on the analysis of the providers, I can now generate the final response:
Provider ordering is correctly implemented
The provider hierarchy is well-structured:
AtlasProvideruses web3 hooks and query client but doesn't depend on other providersIsListViewProvideris a simple state provider with no external dependencies- Other providers weren't found in the codebase, suggesting they might be simple state containers or have been removed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential dependencies between AtlasProvider and other providers
# Search for useWeb3 usage in AtlasProvider
echo "Checking AtlasProvider's dependencies on Web3Provider..."
rg -l "useWeb3" "web/src/context/AtlasProvider"
# Search for useQueryClient usage in AtlasProvider
echo "Checking AtlasProvider's dependencies on QueryClientProvider..."
rg -l "useQueryClient" "web/src/context/AtlasProvider"
Length of output: 1231
Script:
#!/bin/bash
# Let's check for dependencies between providers
# Check AtlasProvider implementation
echo "Checking AtlasProvider implementation..."
fd "AtlasProvider" --exec cat {}
# Check if other providers use Atlas context
echo -e "\nChecking usage of Atlas context in other providers..."
rg "useAtlas" "web/src/context"
# Check for imports in provider files
echo -e "\nChecking provider imports..."
fd "Provider" "web/src/context" --exec rg "import.*from" {}
Length of output: 12293
Script:
#!/bin/bash
# Let's check for dependencies between the remaining providers
# Check IsListViewProvider implementation
echo "Checking IsListViewProvider implementation..."
fd "IsListViewProvider" --exec cat {}
# Check SubmitListProvider implementation
echo -e "\nChecking SubmitListProvider implementation..."
fd "SubmitListProvider" --exec cat {}
# Check SubmitItemProvider implementation
echo -e "\nChecking SubmitItemProvider implementation..."
fd "SubmitItemProvider" --exec cat {}
# Check RegistryDetailsProvider implementation
echo -e "\nChecking RegistryDetailsProvider implementation..."
fd "RegistryDetailsProvider" --exec cat {}
Length of output: 1534
web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/EmailVerificationInfo.tsx (1)
64-64: Verify Atlas context integration.
Let's ensure that all required methods from AtlasProvider are properly typed and available.
✅ Verification successful
Atlas context integration is properly implemented
The AtlasProvider context is correctly implemented with all required methods:
userExistsis defined as a memoized value that checks user's email existenceuseris available through a React Query hook with proper typingupdateEmailis implemented as an async function with proper error handling and typing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify AtlasProvider context implementation
# Expected: Find the context definition with proper types for userExists, user, and updateEmail
# Search for AtlasProvider context definition
ast-grep --pattern 'const AtlasProvider = ({ children }: { children: ReactNode }) => {
$$$
}'
# Search for type definitions
rg -A 5 'interface AtlasContextType'
Length of output: 136
Script:
#!/bin/bash
# Let's try different search patterns to locate the Atlas context implementation
# Search for AtlasProvider or Atlas context related files
fd -e ts -e tsx atlas -i
# Search for useAtlasProvider hook definition
ast-grep --pattern 'const useAtlasProvider = () => {
$$$
}'
# Search for createContext with Atlas
rg -B 2 -A 5 "createContext.*Atlas"
# Search for Atlas related type definitions or interfaces
rg -B 2 -A 5 "type.*Atlas|interface.*Atlas"
Length of output: 1643
Script:
#!/bin/bash
# Let's check the complete interface definition and implementation
# Get the full content of AtlasProvider.tsx to verify the required methods
cat web/src/context/AtlasProvider.tsx
# Specifically look for updateEmail implementation
ast-grep --pattern 'const updateEmail = async ($$$) => {
$$$
}'
# Look for userExists implementation
ast-grep --pattern 'const userExists = $$$'
Length of output: 10743
web/src/pages/SubmitItem/index.tsx (2)
Line range hint 1-93: Consider integrating with AtlasProvider for enhanced user management.
Given the PR's focus on Atlas integration for user management and API interactions, consider leveraging the new AtlasProvider context in this component for:
- User authentication state management (currently using only wallet connection)
- User permissions verification for item submission
- API interactions for registry details
Let's check if other components are already using AtlasProvider:
#!/bin/bash
# Description: Check how AtlasProvider is being used in other components
# Expect: Find patterns of AtlasProvider usage for authentication
# Search for AtlasProvider imports and usage
rg -A 5 "useAtlasProvider|AtlasProvider"14-14: LGTM! Verify import consistency across the codebase.
The change from named to default import aligns with the standardization effort.
Let's verify the consistency of EnsureAuth imports across the codebase:
✅ Verification successful
Import style is consistent across the codebase
The verification shows that all imports of EnsureAuth are using the default import style:
import EnsureAuth from "components/EnsureAuth";This pattern is consistently used in all files:
- web/layout/Header/navbar/Menu/Settings/Notifications/index.tsx
- web/pages/SubmitItem/index.tsx
- web/pages/SubmitList/index.tsx
- web/components/ActionButton/Modal/Buttons/index.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining named imports of EnsureAuth
# Expect: All imports should use the default import style
# Search for any remaining named imports
rg "import.*{.*EnsureAuth.*}.*from.*components/EnsureAuth"
# Compare with default imports to ensure complete migration
rg "import EnsureAuth from.*components/EnsureAuth"
Length of output: 13420
web/src/components/ActionButton/Modal/EvidenceUpload.tsx (1)
8-9: LGTM: Clean integration of AtlasProvider
The new imports align well with the PR's objective of centralizing state management through AtlasProvider.
web/package.json (3)
43-43: Review build scripts after architecture changes.
With the removal of serverless functions and Supabase integration, verify that the remaining scripts, particularly generate:hooks, are still correctly configured and don't have hidden dependencies on the removed functionality.
#!/bin/bash
# Check for any remaining references to removed functionality
echo "Checking for any lingering references to removed systems..."
rg -l "supabase|netlify/functions|filebase" "src/"Line range hint 1-124: Missing Atlas integration dependencies.
The PR objectives mention integration of AtlasProvider for user management and API calls, but there are no corresponding dependencies added to support this integration. Please:
- Add required Atlas-related dependencies
- Document any environment variables or configuration needed
- Update the README with Atlas integration details
#!/bin/bash
# Check for Atlas-related configuration
echo "Checking for Atlas configuration files..."
fd -t f "atlas|.env"
# Check for Atlas imports in the codebase
echo "Checking for Atlas usage without dependencies..."
rg -l "AtlasProvider|atlas" "src/"Line range hint 2-2: Verify the major version bump implications.
The version bump to 2.0.0 indicates breaking changes. Please ensure:
- All consumers of this package are notified
- Migration guide is documented
- CHANGELOG.md is updated
web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx (1)
60-64: LGTM! Clean and efficient implementation.
The useEffect hook is well-structured with proper dependency tracking and early return pattern.
web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
web/src/layout/Header/navbar/Menu/Settings/index.tsx (2)
69-70: Consider adding prop type validation for initialTab.While the implementation is correct, adding runtime validation for the initialTab prop would improve robustness.
const Settings: React.FC<ISettings> = ({ toggleIsSettingsOpen, initialTab }) => { + if (initialTab !== undefined && (initialTab < 0 || initialTab >= TABS.length)) { + console.warn(`Invalid initialTab value: ${initialTab}. Defaulting to 0.`); + } const [currentTab, setCurrentTab] = useState<number>(initialTab || 0);
69-77: Consider separating navigation logic from UI state management.The current implementation mixes navigation concerns with UI state management. Consider extracting the navigation logic into a custom hook for better separation of concerns and reusability:
// useSettingsNavigation.ts const useSettingsNavigation = (onClose: () => void) => { const location = useLocation(); const navigate = useNavigate(); const handleClose = () => { onClose(); if (location.hash.includes("#notifications")) { navigate("#", { replace: true }); } }; return { handleClose }; };This would simplify the Settings component and make the navigation logic more maintainable and testable.
web/src/layout/Header/DesktopHeader.tsx (1)
96-100: Consider improving the URL fragment handling implementation.A few suggestions to enhance maintainability and performance:
- Extract the tab index as a named constant
- Consider using a more descriptive function name
- Optimize the useCallback dependencies
Consider applying these changes:
+const NOTIFICATIONS_TAB_INDEX = 1; + -const initializeFragmentURL = useCallback(() => { +const initializeNotificationsFromURL = useCallback(() => { const hasNotificationsPath = location.hash.includes("#notifications"); toggleIsSettingsOpen(hasNotificationsPath); - setInitialTab(hasNotificationsPath ? 1 : 0); + setInitialTab(hasNotificationsPath ? NOTIFICATIONS_TAB_INDEX : 0); -}, [toggleIsSettingsOpen, location.hash]); +}, [toggleIsSettingsOpen]);web/src/pages/Settings/EmailConfirmation/index.tsx (4)
138-138: Remove unnecessary variable in destructuring assignmentThe underscore
_in the destructuring assignment is unnecessary since onlysearchParamsis used. Removing it can simplify the code.Consider updating the code as:
- const [searchParams, _] = useSearchParams(); + const [searchParams] = useSearchParams();
60-65: Simplify styled-component interpolationThe nested interpolations in the
Headerstyled-component can be simplified to enhance readability.You can refactor the code as follows:
- const Header = styled.h1<{ fontColor: string }>` - ${textCss} - ${({ fontColor }) => css` - color: ${({ theme }) => theme[fontColor]}; - `}; - `; + const Header = styled.h1<{ fontColor: string }>` + ${textCss} + color: ${({ theme, fontColor }) => theme[fontColor]}; + `;
72-82: Simplify interpolation inHeaderIconContainerSimilar to the previous comment, the interpolation within
HeaderIconContainercan be streamlined for better maintainability.Consider refactoring the code like this:
- const HeaderIconContainer = styled.div<{ iconColor: string }>` - svg { - width: 64px; - height: 64px; - ${({ iconColor }) => css` - path { - fill: ${({ theme }) => theme[iconColor]}; - } - `} - } - `; + const HeaderIconContainer = styled.div<{ iconColor: string }>` + svg { + width: 64px; + height: 64px; + path { + fill: ${({ theme, iconColor }) => theme[iconColor]}; + } + } + `;
86-92: Make SVG icons responsiveThe SVG icons have fixed dimensions, which may not adapt well to different screen sizes. Making them responsive can enhance the user experience.
Consider updating the code to use relative units or responsive styling:
- svg { - width: 250px; - height: 250px; - path { - fill: ${({ theme }) => theme.whiteBackground}; - } - } + svg { + width: 100%; + height: auto; + max-width: 250px; + path { + fill: ${({ theme }) => theme.whiteBackground}; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
web/src/layout/Header/DesktopHeader.tsx(3 hunks)web/src/layout/Header/navbar/Menu/Settings/index.tsx(2 hunks)web/src/layout/Header/navbar/index.tsx(1 hunks)web/src/pages/Settings/EmailConfirmation/index.tsx(1 hunks)
🔇 Additional comments (8)
web/src/layout/Header/navbar/Menu/Settings/index.tsx (2)
9-9: LGTM: Router hooks import added for navigation.
The addition of useLocation and useNavigate hooks is appropriate for the new navigation requirements.
72-77: Verify the navigation behavior on settings close.
The click-away handler now includes navigation logic that might affect the user experience. Please ensure:
- The navigation to "#" doesn't cause unwanted page reloads
- The replace:true option properly handles the browser history
✅ Verification successful
Hash navigation implementation is safe and consistent
The verification results show that:
- The hash-based navigation is only used in this specific context for notifications (#notifications)
- The
replace: trueoption is appropriate as it prevents adding a new history entry for this UI-only state - Other
useClickAwayhandlers in the codebase don't interfere with hash navigation - The navigation pattern aligns with React Router's expected behavior and won't cause page reloads
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might be affected by the hash navigation
rg -l "#notifications" --type ts --type tsx
# Look for other hash-based navigation patterns that might conflict
rg "navigate\(.[#]" --type ts --type tsx
Length of output: 144
Script:
#!/bin/bash
# Search for other components that might be affected by the hash navigation
rg -l "#notifications"
# Look for other hash-based navigation patterns that might conflict
rg -A 2 'navigate\(.*#'
# Check for useLocation hooks to understand hash handling
rg -A 2 'useLocation'
# Look for other click-away handlers
ast-grep --pattern 'useClickAway($_, $_)'
Length of output: 9107
web/src/layout/Header/navbar/index.tsx (1)
56-56: LGTM! Verify Settings component implementation.
The addition of the optional initialTab property to the ISettings interface is clean and follows TypeScript best practices. However, let's verify that the Settings component properly handles this new property.
✅ Verification successful
The initialTab prop is properly implemented and used correctly
The Settings component in web/src/layout/Header/navbar/Menu/Settings/index.tsx correctly implements the initialTab prop, using it to initialize the currentTab state with a fallback to 0. The prop is also properly passed from the DesktopHeader component where it's managed via useState.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Settings component implementation and usage
# Expected: Find the Settings component implementation and verify it handles the initialTab prop
# Look for the Settings component implementation
ast-grep --pattern 'const Settings = ({ toggleIsSettingsOpen, initialTab }: ISettings) => {
$$$
}'
# Look for any Settings component usage to ensure proper prop passing
rg -A 2 '<Settings.*?/>'
Length of output: 846
Script:
#!/bin/bash
# Let's find the Settings component implementation and its usage of initialTab
fd "Settings" --type f --exec rg -l "Settings" {} \; | xargs rg -A 10 "const Settings.*=.*{"
# Also check for any TabPanel or similar tab-related components that might be using initialTab
rg -A 5 "initialTab.*="
Length of output: 1369
web/src/layout/Header/DesktopHeader.tsx (3)
1-1: LGTM: Clean state management implementation.
The state initialization and import organization follow React best practices.
Also applies to: 92-92
102-102: LGTM: Proper effect hook implementation.
The useEffect is correctly implemented with appropriate dependencies.
142-142: Verify Settings component prop types.
The initialTab prop has been added to the Settings component. Let's verify that the prop types are properly defined.
✅ Verification successful
Let me gather more information to verify the Settings component implementation.
Based on the verification results, I can now provide the final response:
Props are correctly typed and implemented in the Settings component
The verification confirms:
ISettingsinterface properly definesinitialTabas an optional number prop- Settings component correctly implements the interface and uses the prop in its initialization
- The prop is properly handled with a fallback value:
initialTab || 0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Settings component properly defines initialTab in its prop types
# Test: Search for Settings component prop types definition
ast-grep --pattern 'interface $INTERFACE_NAME {
$$$
initialTab?: number;
$$$
}'
# Test: Search for Settings component implementation
ast-grep --pattern 'const Settings = ({ $$$, initialTab $$ }: $PROPS_TYPE) => {'
Length of output: 428
Script:
#!/bin/bash
# Search for Settings component implementation and its usage of initialTab
rg "const Settings.*initialTab" -A 5
# Search for any other interfaces that might be extending ISettings
ast-grep --pattern 'interface $_ extends ISettings {
$$$
}'
# Search for Settings component file to verify its imports and implementation
fd -e tsx -e ts "Settings" --exec cat {}
Length of output: 760
web/src/pages/Settings/EmailConfirmation/index.tsx (2)
142-154: Enhance error handling and effect dependencies
The previous review comment about improving the useEffect implementation is still valid and applicable to the current code segment.
157-161: Verify message configuration logic
The conditions in the useMemo hook may lead to incorrect messages being displayed. For example, when isTokenInvalid is true, it returns messageConfigs.invalid, but it might be intended to show messageConfigs.expired.
Please verify that the conditions align with the desired messaging logic. You might need to adjust the conditions or add additional checks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
web/src/context/SubmitItemContext.tsx (2)
55-55: Consider using early return pattern for better readability.The current condition structure can be made more clear using an early return pattern.
-if (location.pathname.includes("/submit-item") || location.pathname.includes("/attachment")) return; +const isSubmissionRoute = location.pathname.includes("/submit-item") || location.pathname.includes("/attachment"); +if (isSubmissionRoute) return;
Based on the evidence gathered, I can now provide the final response:
Migration to sessionStorage is indeed part of the ongoing changes
The codebase shows an active migration pattern from localStorage to sessionStorage:
- A new
useSessionStoragehook has been implemented with identical interface touseLocalStorageAtlasProvideris already usinguseSessionStoragefor auth token storage- Several components still use
useLocalStorage, includingSubmitItemContextThe review comment is correct - this component should be migrated to use
useSessionStoragefor consistency with the broader changes being made in the PR.🔗 Analysis chain
Line range hint
1-80: Consider migrating from localStorage to sessionStorage.The PR objectives mention replacing local storage with session storage. However, this component still uses
useLocalStorage. Consider migrating touseSessionStoragefor consistency with the broader changes being made in this PR.Let's verify the storage migration pattern across the codebase:
Let me try the search again with correct command syntax:
Let me try with a different command syntax that should work in the sandbox environment:
Let me check the content of the storage hooks to understand the migration pattern:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for remaining localStorage usage and new sessionStorage patterns echo "Checking for remaining localStorage usage:" rg -i "localStorage" --type tsx --type ts echo -e "\nChecking for new sessionStorage patterns:" rg -i "sessionStorage" --type tsx --type tsLength of output: 359
Script:
#!/bin/bash # Check for remaining localStorage usage and new sessionStorage patterns echo "Checking for localStorage usage:" rg -i "localStorage" -t ts -t tsx echo -e "\nChecking for sessionStorage usage:" rg -i "sessionStorage" -t ts -t tsx # Also check for specific hook usage echo -e "\nChecking for storage hook imports and definitions:" rg "use(Local|Session)Storage" -t ts -t tsxLength of output: 476
Script:
#!/bin/bash # Search for storage-related patterns using find and grep echo "Checking for localStorage usage:" find . -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l -i "localStorage" {} \; echo -e "\nChecking for sessionStorage usage:" find . -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l -i "sessionStorage" {} \; echo -e "\nChecking for storage hook definitions:" find . -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "useLocalStorage\|useSessionStorage" {} \;Length of output: 1201
Script:
#!/bin/bash echo "Content of useLocalStorage hook:" cat ./web/src/hooks/useLocalStorage.ts echo -e "\nContent of useSessionStorage hook:" cat ./web/src/hooks/useSessionStorage.ts echo -e "\nChecking AtlasProvider for migration pattern:" cat ./web/src/context/AtlasProvider.tsxLength of output: 11872
web/src/context/SubmitListContext.tsx (2)
Line range hint
39-40: Consider migrating from localStorage to sessionStorage.According to the PR objectives, there's an initiative to replace localStorage with sessionStorage. This component still uses the
useLocalStoragehook.Apply this change:
-import { useLocalStorage } from "hooks/useLocalStorage"; +import { useSessionStorage } from "hooks/useSessionStorage"; // ... and update the hook usage: -const [listData, setListData] = useLocalStorage<IListData> +const [listData, setListData] = useSessionStorage<IListData> -const [listMetadata, setListMetadata] = useLocalStorage<IListMetadata> +const [listMetadata, setListMetadata] = useSessionStorage<IListMetadata>Also applies to: 144-147
144-147: Enhance cleanup logic with explicit route handling.The current cleanup logic might be too broad. Consider implementing specific cleanup behaviors for different routes.
Consider refactoring to:
-if (location.pathname.includes("/submit-list") || location.pathname.includes("/attachment")) return; - -resetListData(); +const shouldPreserveData = ["/submit-list", "/attachment"].some(route => + location.pathname.includes(route) +); + +if (!shouldPreserveData) { + resetListData(); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/src/context/SubmitItemContext.tsx(1 hunks)web/src/context/SubmitListContext.tsx(1 hunks)
🔇 Additional comments (2)
web/src/context/SubmitItemContext.tsx (1)
55-57: Verify cleanup behavior with attachment routes.
The cleanup condition now includes the "/attachment" route. Please ensure this doesn't cause premature data cleanup when users are in the middle of the attachment flow.
✅ Verification successful
Let me try a different search strategy to understand the attachment flow better.
Let me search for the attachment flow implementation to understand how it's connected to the submit item flow.
Based on the findings, I can now provide a final response:
The attachment route exclusion is correctly implemented
The code shows that:
- The attachment route (
/attachment) is a sibling route to submit-item, shown inapp.tsx - Both
SubmitItemContextandSubmitListContextpreserve their data during attachment flows - Data cleanup only occurs when navigating away from both submit and attachment routes
- The
resetItemDatais only called after successful submission or when completely leaving these flows
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for attachment-related route definitions and components
echo "Checking attachment-related routes:"
rg -i "path.*attachment" --type tsx --type ts
echo -e "\nChecking components that might be affected:"
ast-grep --pattern 'const $_ = () => {
$$$
location.pathname.includes("/attachment")
$$$
}'
Length of output: 359
Script:
#!/bin/bash
# Search for attachment-related routes and components with broader patterns
echo "Checking attachment route patterns:"
rg -i "attachment" --type ts --type tsx -A 5 -B 5
echo -e "\nChecking SubmitItemContext usage:"
rg -l "SubmitItemContext"
echo -e "\nChecking resetItemData usage:"
rg "resetItemData" -A 3 -B 3
Length of output: 4637
Script:
#!/bin/bash
# Look for attachment-related components and routes in the web directory
echo "Searching for attachment-related files:"
fd -e tsx -e ts . web/src | xargs rg -l "attachment"
echo -e "\nChecking router configuration:"
rg -i "route.*path.*submit" web/src -A 5 -B 5
echo -e "\nChecking for attachment state management:"
rg "attachment" web/src/context -A 5 -B 5
Length of output: 3211
web/src/context/SubmitListContext.tsx (1)
144-144: Verify cleanup behavior for attachment route.
While preserving local storage for the attachment route is logical, there should be a mechanism to clean up this data after attachment handling is complete.
Run the following script to verify attachment handling cleanup:
✅ Verification successful
Cleanup behavior for attachment route is properly implemented
The current implementation is correct and safe because:
- The
resetListData()function properly cleans up by resetting bothlistDataandlistMetadatato their initial states - The cleanup logic is intentionally skipped for both "/submit-list" and "/attachment" routes to preserve necessary state
- When navigating away from these routes, the cleanup is automatically triggered via the
useEffecthook, ensuring no memory leaks
The preservation of state during attachment handling is a deliberate design choice, and the cleanup mechanism is properly implemented through the route-based cleanup strategy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for cleanup mechanisms in attachment handling
# Test: Look for cleanup calls in attachment-related code
rg -A 5 "attachment.*cleanup|cleanup.*attachment"
# Test: Check attachment route implementation
ast-grep --pattern 'const.*Route.*attachment.*{
$$$
}'
Length of output: 110
Script:
#!/bin/bash
# Let's try a broader search to understand the attachment handling flow
# Check for attachment-related code
rg -l "attachment" --type tsx --type ts
# Look for local storage operations
rg "localStorage\." -A 5 web/src/context/SubmitListContext.tsx
# Search for cleanup or reset functions
rg "(cleanup|reset|clear).*storage" -A 5 web/src/context/SubmitListContext.tsx
# Check for useEffect hooks that might handle cleanup
ast-grep --pattern 'useEffect(() => {
$$$
}, [$$$])' web/src/context/SubmitListContext.tsx
Length of output: 1037
Script:
#!/bin/bash
# Let's check the resetListData implementation and other related functions
# Look for resetListData implementation
ast-grep --pattern 'const resetListData = $$$'
# Check for any other cleanup mechanisms in the component
rg "resetListData" -A 5 web/src/context/SubmitListContext.tsx
# Look for attachment-related state management
rg "attachment.*state|state.*attachment" web/src/context/SubmitListContext.tsx
Length of output: 1510
PR-Codex overview
This PR focuses on refactoring and enhancing the codebase by removing unused files, implementing new utility functions, and improving user authentication and settings management. It also introduces a new
AtlasProvidercontext for better state management.Detailed summary
isEmptyutility function to check if a string is empty.EnsureAuthto utilizeAtlasProvider.Settingspage to manage email confirmation.uploadToIpfsfunctionality with role-based uploads.AtlasProvider.Summary by CodeRabbit
New Features
AtlasProviderfor enhanced user authentication and interactions.EmailVerificationInfocomponent to display email verification status and resend option.EmailConfirmationcomponent for handling email verification processes.Settingspage with nested routing for email confirmation.loginUser,getNonce,createMessage,addUser,fetchUser,updateEmail, andconfirmEmailfunctionalities in the Atlas utilities.Bug Fixes
Refactor
uploadFilefunction fromAtlasProvider.FormContactandImageInputcomponents.Chores
package.jsonto reflect version changes and removed unused dependencies.