-
-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
saves the current step of verification form to localStorage #4929
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes modify the Changes
Sequence DiagramsequenceDiagram
participant Browser
participant VerificationProvider
participant LocalStorage
VerificationProvider->>LocalStorage: Check stored step
alt Stored step exists
LocalStorage-->>VerificationProvider: Return stored step
VerificationProvider->>VerificationProvider: Set current step
else No stored step
VerificationProvider->>VerificationProvider: Determine step from verification data
end
VerificationProvider->>LocalStorage: Store current step when changed
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/context/verification.context.tsx (2)
53-68
: Consider adding expiration and error handling to localStorage operationsThe implementation could be enhanced with:
- An expiration mechanism to prevent stale data persistence
- Error handling for parseInt and localStorage operations
- A project-specific key prefix to prevent naming conflicts
Consider this improved implementation:
const storeStepInLocalStorage = (step: number) => { if (typeof window !== 'undefined') { - localStorage.setItem('lastStep', step.toString()); + const data = { + step, + timestamp: Date.now(), + projectSlug: slug + }; + try { + localStorage.setItem('verification_lastStep', JSON.stringify(data)); + } catch (error) { + console.error('Failed to store step:', error); + } } }; const getStepFromLocalStorage = () => { if (typeof window !== 'undefined') { - const lastStep = localStorage.getItem('lastStep'); - return lastStep ? parseInt(lastStep, 10) : -1; + try { + const data = localStorage.getItem('verification_lastStep'); + if (!data) return -1; + + const parsed = JSON.parse(data); + // Return -1 if data is older than 24 hours or belongs to different project + if (Date.now() - parsed.timestamp > 24 * 60 * 60 * 1000 || + parsed.projectSlug !== slug) { + localStorage.removeItem('verification_lastStep'); + return -1; + } + return typeof parsed.step === 'number' ? parsed.step : -1; + } catch { + return -1; + } } return -1; };
126-131
: Optimize localStorage writes with cleanup and debouncingThe current implementation might cause unnecessary writes to localStorage. Consider adding cleanup and debouncing.
Here's an optimized version:
useEffect(() => { + const debounceTimeout = 1000; // 1 second + let timeoutId: NodeJS.Timeout; + if (step >= 0) { - storeStepInLocalStorage(step); + timeoutId = setTimeout(() => { + storeStepInLocalStorage(step); + }, debounceTimeout); } + + return () => { + if (timeoutId) { + clearTimeout(timeoutId); + } + }; }, [step]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/context/verification.context.tsx
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/context/verification.context.tsx (2)
Line range hint
53-132
: Implementation successfully meets PR objectivesThe changes effectively implement step persistence using localStorage, meeting the goal of preserving user progress during page navigation or refresh. While there are opportunities for enhancement, the core functionality is solid and well-implemented.
80-85
: Verify stored step validity against current form stateThe current implementation might restore an invalid step if the form state has changed since it was stored. Consider validating the stored step against the current verification state.
Add validation before using the stored step:
let lastStep = getStepFromLocalStorage(); -if (lastStep !== -1) { +if (lastStep !== -1 && lastStep <= menuList.length) { + // Ensure the step is valid for current verification state + const isStepValid = lastStep === 0 || ( + projectVerification.emailConfirmed && + lastStep <= findFirstIncompleteStep(menuList, projectVerification) + ); + if (isStepValid) { setStep(lastStep); return; + } }
@kkatusic could you please check this Pr? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maryjaf nice job, but it now returns to one step before :(
I've tested on this branch "https://giveth-dapps-v2-git-edit-in-socialprofile-page-general-magic.vercel.app/" and it works as expected, could you please add more data about your scenario, Thanks@kkatusic Screen.Recording.2025-01-15.at.11.17.21.AM.mov |
@maryjaf, check my video, after accepting Discord to connect I was returning to previous step. I tested on same link as you share. If you can't get same problem, we can schedule the call and then check it together. Screen.Recording.2025-01-15.at.10.31.01.mov |
#4924
Summary by CodeRabbit