refactor: abstract local storage logic into custom hook and harden file validation#489
Open
tirth707 wants to merge 3 commits intoAOSSIE-Org:mainfrom
Open
refactor: abstract local storage logic into custom hook and harden file validation#489tirth707 wants to merge 3 commits intoAOSSIE-Org:mainfrom
tirth707 wants to merge 3 commits intoAOSSIE-Org:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
actor User
participant TextInput as Text_Input Component
participant Hook as useQuizHistory Hook
participant LS as localStorage
participant Server
User->>TextInput: Select file or enter Google Doc URL
TextInput->>TextInput: Validate input (MIME/ext/size) and enforce exclusivity
alt File invalid
TextInput-->>User: Show validation error
else File valid
TextInput->>Server: Start XHR upload
Server-->>TextInput: progress events
TextInput->>TextInput: Update progress & speed UI
Server-->>TextInput: upload complete (response with extracted quiz)
TextInput->>Hook: addQuiz(quizDetails)
Hook->>LS: write updated quizHistory
LS-->>Hook: acknowledge write
Hook-->>TextInput: updated history
TextInput-->>User: Display result / enable Next
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eduaid_web/src/hooks/useQuizHistory.js`:
- Around line 6-7: The deserialized value from localStorage in useQuizHistory
(const saved = localStorage.getItem("quizHistory")) may not be an array and can
break the spread usage later; after JSON.parse(saved) validate that the parsed
value is an Array (Array.isArray(parsed)) and if not, fallback to an empty array
(and optionally log a warning), then return that validated array so subsequent
code that spreads quizHistory (used in the hook’s update logic) always works
safely.
- Around line 14-19: The addQuiz function closes over history causing stale
updates; change setHistory to use a functional updater that receives
prevHistory, compute updatedHistory = [quiz, ...prevHistory].slice(0,5), call
setHistory(updatedHistory) and then write JSON.stringify(updatedHistory) to
localStorage (localStorage.setItem("quizHistory", ...)) so rapid consecutive
addQuiz calls never lose entries.
In `@eduaid_web/src/pages/Text_Input.jsx`:
- Around line 98-144: Add an XHR timeout and an ontimeout handler so stalled
uploads don't leave isUploading true: set xhr.timeout to a sensible value (e.g.,
30000) before xhr.send(formData), implement xhr.ontimeout to call
setIsUploading(false), setUploadError("Upload timed out."), setUploadProgress(0)
and setUploadSpeed(""), and if desired abort the request via xhr.abort(); ensure
these changes reference the existing xhr, setIsUploading, setUploadError,
setUploadProgress, and setUploadSpeed identifiers.
- Around line 13-20: The formatBytes function can produce a negative index when
0 < bytes < 1 because i = Math.floor(Math.log(bytes)/Math.log(1024)) can be -1;
update the logic in formatBytes to clamp i to the valid range (0 ..
sizes.length-1) before using it (e.g., set i = Math.max(0, Math.min(i,
sizes.length - 1))) so sizes[i] never reads an out-of-range index; keep the
existing bytes === 0 early return and ensure the clamp happens after computing
i.
- Around line 64-72: The backend is missing robust server-side validation for
uploads; update the /upload endpoint handler in backend/server.py to enforce a
MAX_UPLOAD_SIZE constant, validate the uploaded file's MIME type using a
reliable library (python-magic or similar) and additionally verify file
signatures (magic bytes) for PDFs/MP3s, sanitize the original filename with a
safe utility (e.g., werkzeug.utils.secure_filename or pathlib normalization) to
prevent directory traversal, and reject unsupported types with explicit error
responses; then update backend/Generator/main.py FileProcessor.process_file to
either add MP3 handling (parsing/extraction) or raise a clear
unsupported-media-type error so the frontend (which currently allows MP3 via
fileInputRef) matches backend capabilities, and ensure all rejections return
non-200 HTTP codes and descriptive messages.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
eduaid_web/eduaid_web@0.1.0eduaid_web/react-scriptseduaid_web/src/hooks/useQuizHistory.jseduaid_web/src/pages/Text_Input.jsx
tirth707
force-pushed
the
refactor/quiz-history-hook-and-validation
branch
from
7bc23b2 to
52642a1
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors localStorage logic into a custom useQuizHistory hook and enhances file upload validation and UX. However, it introduces several critical bugs that will break existing functionality.
Changes:
- Created
useQuizHistorycustom hook to centralize quiz history management - Enhanced file upload with progress tracking, speed display, and improved validation
- Improved state synchronization between file upload and Google Doc URL inputs
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
| eduaid_web/src/hooks/useQuizHistory.js | New custom hook for managing quiz history in localStorage with error handling |
| eduaid_web/src/pages/Text_Input.jsx | Major refactor with XHR-based file upload, progress tracking, enhanced validation, and hook integration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
eduaid_web/src/pages/Text_Input.jsx (2)
13-20:⚠️ Potential issue | 🟡 MinorClamp
formatByteslower bound to prevent invalid unit lookup.For sub-byte values,
ican become-1, producing an invalid unit label. Clamp both bounds before indexingsizes.Suggested fix
const formatBytes = (bytes) => { - if (bytes === 0) return '0 Bytes'; + if (bytes <= 0) return "0 Bytes"; const k = 1024; - const sizes = ['Bytes', 'KB', 'MB', 'GB', 'TB', 'PB']; + const sizes = ["Bytes", "KB", "MB", "GB", "TB", "PB"]; let i = Math.floor(Math.log(bytes) / Math.log(k)); - if (i >= sizes.length) i = sizes.length - 1; - return parseFloat((bytes / Math.pow(k, i)).toFixed(2)) + ' ' + sizes[i]; + i = Math.max(0, Math.min(i, sizes.length - 1)); + return `${parseFloat((bytes / Math.pow(k, i)).toFixed(2))} ${sizes[i]}`; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eduaid_web/src/pages/Text_Input.jsx` around lines 13 - 20, The formatBytes function can compute i = -1 for values less than 1 and then index sizes[-1]; clamp i before using it. In the formatBytes function, after computing let i = Math.floor(Math.log(bytes) / Math.log(k)); ensure i is bounded to the valid range (0 .. sizes.length - 1) (e.g., set i = Math.max(0, Math.min(i, sizes.length - 1))) so the sizes array lookup (sizes[i]) cannot be out of range.
98-144:⚠️ Potential issue | 🟠 MajorAdd XHR timeout handling so upload UI cannot get stuck indefinitely.
A stalled request can keep
isUploadingtrue for too long. Add timeout +ontimeoutstate reset.Suggested fix
const xhr = new XMLHttpRequest(); xhr.open("POST", `${process.env.REACT_APP_API_URL || ""}/upload_endpoint`); + xhr.timeout = 30000; @@ xhr.onerror = () => { setUploadError("Network error. Is the backend running?"); setUploadProgress(0); setUploadSpeed(""); setIsUploading(false); }; + + xhr.ontimeout = () => { + setUploadError("Upload timed out. Please try again."); + setUploadProgress(0); + setUploadSpeed(""); + setIsUploading(false); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eduaid_web/src/pages/Text_Input.jsx` around lines 98 - 144, The XHR upload lacks a timeout so a stalled request can leave isUploading true indefinitely; set xhr.timeout (e.g., 60000) and add an xhr.ontimeout handler that calls setIsUploading(false), resets setUploadProgress(0), clears setUploadSpeed(""), and sets a useful setUploadError message (e.g., "Upload timed out"); add this next to the existing xhr.onerror and xhr.onload handlers where xhr is created and configured.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eduaid_web/src/pages/Text_Input.jsx`:
- Around line 64-72: The current validation allows files with a matching
extension even when the browser reports a conflicting MIME; update the check so
both the extension and an acceptable MIME are required: compute
hasAllowedExtension as before and require (allowedTypes.has(file.type) ||
file.type === "" || file.type === "application/octet-stream") to be true as
well; replace the if condition that sets setUploadError and clears
fileInputRef.current.value with one that rejects when NOT (hasAllowedExtension
&& acceptableMime), keeping the same error message and clearing behavior;
reference variables/functions: allowedTypes, hasAllowedExtension, file.type,
setUploadError, fileInputRef.
---
Duplicate comments:
In `@eduaid_web/src/pages/Text_Input.jsx`:
- Around line 13-20: The formatBytes function can compute i = -1 for values less
than 1 and then index sizes[-1]; clamp i before using it. In the formatBytes
function, after computing let i = Math.floor(Math.log(bytes) / Math.log(k));
ensure i is bounded to the valid range (0 .. sizes.length - 1) (e.g., set i =
Math.max(0, Math.min(i, sizes.length - 1))) so the sizes array lookup (sizes[i])
cannot be out of range.
- Around line 98-144: The XHR upload lacks a timeout so a stalled request can
leave isUploading true indefinitely; set xhr.timeout (e.g., 60000) and add an
xhr.ontimeout handler that calls setIsUploading(false), resets
setUploadProgress(0), clears setUploadSpeed(""), and sets a useful
setUploadError message (e.g., "Upload timed out"); add this next to the existing
xhr.onerror and xhr.onload handlers where xhr is created and configured.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
eduaid_web/src/hooks/useQuizHistory.jseduaid_web/src/pages/Text_Input.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- eduaid_web/src/hooks/useQuizHistory.js
tirth707
force-pushed
the
refactor/quiz-history-hook-and-validation
branch
from
52642a1 to
c5559f6
Compare
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
eduaid_web/src/pages/Text_Input.jsx (1)
65-72:⚠️ Potential issue | 🟠 MajorRequire extension + acceptable MIME together during file validation.
Current condition allows conflicting MIME types when extension matches (and vice versa). This keeps bypass paths open in client validation.
Suggested fix
- const allowedTypes = new Set(['application/pdf', 'audio/mpeg']); + const allowedTypes = new Set(["application/pdf", "audio/mpeg"]); const hasAllowedExtension = /\.(pdf|mp3)$/i.test(file.name || ""); + const mimeType = (file.type || "").toLowerCase(); + const acceptableMime = + allowedTypes.has(mimeType) || + mimeType === "" || + mimeType === "application/octet-stream"; - if (!allowedTypes.has(file.type) && !hasAllowedExtension) { + if (!(hasAllowedExtension && acceptableMime)) { setUploadError("Unsupported file type. Please select a PDF or MP3 file."); if (fileInputRef.current) fileInputRef.current.value = ""; return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eduaid_web/src/pages/Text_Input.jsx` around lines 65 - 72, The current validation uses an OR between MIME and extension which allows mismatches; change it to require both a valid MIME and a matching extension by checking that allowedTypes.has(file.type) AND hasAllowedExtension are true before accepting the file. If the combined check fails, call setUploadError("Unsupported file type. Please select a PDF or MP3 file."), reset fileInputRef.current.value if present, and return. Update the validation logic around allowedTypes, hasAllowedExtension, setUploadError and fileInputRef in Text_Input.jsx accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eduaid_web/src/hooks/useQuizHistory.js`:
- Around line 16-23: The addQuiz state updater currently performs a side effect
(localStorage.setItem) inside the setHistory functional updater and is wrapped
by a try/catch that cannot catch async updater errors; make the updater pure by
changing addQuiz to only call setHistory((prev) => [quiz, ...prev].slice(0,5))
and remove localStorage from inside that callback, then add a useEffect that
watches history and performs localStorage.setItem(JSON.stringify(history)) with
its own try/catch to handle persistence errors; reference the addQuiz function,
setHistory updater, history state variable, and a new useEffect([history]) for
locating where to apply the changes.
---
Duplicate comments:
In `@eduaid_web/src/pages/Text_Input.jsx`:
- Around line 65-72: The current validation uses an OR between MIME and
extension which allows mismatches; change it to require both a valid MIME and a
matching extension by checking that allowedTypes.has(file.type) AND
hasAllowedExtension are true before accepting the file. If the combined check
fails, call setUploadError("Unsupported file type. Please select a PDF or MP3
file."), reset fileInputRef.current.value if present, and return. Update the
validation logic around allowedTypes, hasAllowedExtension, setUploadError and
fileInputRef in Text_Input.jsx accordingly.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
eduaid_web/src/hooks/useQuizHistory.js (1)
1-37: Well-structured hook that addresses all prior feedback.The implementation correctly:
- Validates parsed localStorage data is an array (line 8)
- Uses functional state updater to avoid stale writes (line 24)
- Moves localStorage persistence to useEffect (lines 15-21)
One minor inconsistency:
clearHistoryexplicitly callslocalStorage.removeItem, but theuseEffectwill immediately run aftersetHistory([])and write"[]"back to localStorage. TheremoveItemcall is effectively a no-op.,
Optional simplification for clearHistory
const clearHistory = () => { - try { - localStorage.removeItem("quizHistory"); - setHistory([]); - } catch (error) { - console.error("Error clearing history:", error); - } + setHistory([]); };This lets the
useEffecthandle persistence consistently. The try/catch inuseEffectalready handles storage errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eduaid_web/src/hooks/useQuizHistory.js` around lines 1 - 37, clearHistory currently calls localStorage.removeItem and then setHistory([]), but useEffect will immediately persist the new empty array back to storage; simplify clearHistory to only call setHistory([]) (and remove the surrounding try/catch and removeItem call) so persistence is handled consistently by the existing useEffect; update the clearHistory function (referenced by name) to be a single-line setter to avoid the no-op removeItem.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@eduaid_web/src/hooks/useQuizHistory.js`:
- Around line 1-37: clearHistory currently calls localStorage.removeItem and
then setHistory([]), but useEffect will immediately persist the new empty array
back to storage; simplify clearHistory to only call setHistory([]) (and remove
the surrounding try/catch and removeItem call) so persistence is handled
consistently by the existing useEffect; update the clearHistory function
(referenced by name) to be a single-line setter to avoid the no-op removeItem.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues:
Fixes #485
Additional Notes:
Context for Reviewers & Mentors:
This Pull Request is a clean, single-commit resubmission to ensure a pristine Git history synced perfectly with
upstream/main. It introduces key architectural and UX improvements while addressing feedback from previous automated reviews:localStoragelogic into a dedicateduseQuizHistorycustom hook. This centralizes state management, implementstry/catcherror boundaries for storage limits, and strictly enforces the project's 5-quiz history cap.handleFileUploadinText_Input.jsxto validate both MIME types (application/pdf,audio/mpeg) and file extensions (.pdf,.mp3). This resolves an edge case where valid files could be incorrectly rejected by browsers sending generic or empty MIME types.Checklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: Google Gemini
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit