Conversation
📝 WalkthroughWalkthroughAdds an Upload component with drag-and-drop, base64 conversion and progress simulation; integrates it into the Home page to navigate to a new visualizer route; adds a new Changes
Sequence DiagramsequenceDiagram
actor User
participant "Upload Component" as Upload
participant "FileReader API" as FileReader
participant "Home Component" as Home
participant Router
participant "Visualizer Page" as Visualizer
User->>Upload: Drop or select image file
activate Upload
Upload->>FileReader: Read file as base64
activate FileReader
FileReader-->>Upload: base64 data URL
deactivate FileReader
Upload->>Upload: Start progress loop (increment until 100%)
Upload-->>Home: onComplete(base64) callback
deactivate Upload
activate Home
Home->>Home: Generate new ID (Date.now().toString())
Home->>Router: Navigate to /visualizer/{id}
deactivate Home
activate Router
Router->>Visualizer: Route to visualizer.$id.tsx
deactivate Router
activate Visualizer
Visualizer->>User: Render visualizer page
deactivate Visualizer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 Tip CodeRabbit can use your project's `stylelint` configuration to improve the quality of CSS code reviews.Add a Stylelint configuration file to your project to customize how CodeRabbit runs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
app/routes/home.tsx (2)
63-65: Remove commented-out debug code before merging.The commented
console.logexample should be removed to keep the codebase clean.<Upload onComplete={handleUploadComplete} /> - {/* <Upload onComplete={(base64Data)=>{ - console.log("upload complete",base64Data) - }}/> */}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/home.tsx` around lines 63 - 65, Remove the commented-out debug snippet that references the Upload component and its onComplete handler (the block with Upload, onComplete and console.log(base64Data)); delete this commented code entirely from app/routes/home.tsx to keep the file clean and avoid leftover debug examples.
83-85: Hardcoded external URL for preview image.This URL points to a specific external deployment. Consider using a local asset or environment-based URL for better maintainability and to avoid broken images if the external resource becomes unavailable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/home.tsx` around lines 83 - 85, The <img> tag in app/routes/home.tsx uses a hardcoded external src which can break; change it to use a local asset or an environment-driven base URL: replace the literal src string on the <img> element with either an imported/local asset (e.g., import or new URL(...) for the preview image) or construct the src from an env var like PREVIEW_BASE_URL + "/projects/1770803585402/rendered.png" and fall back to a bundled local placeholder; update the <img> element reference accordingly to use the new variable instead of the hardcoded URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/routes/home.tsx`:
- Around line 19-25: handleUploadComplete is discarding the base64Image (the
uploaded floor plan) before navigation, so the visualizer page cannot access it;
update handleUploadComplete to persist the image before navigating—either pass
it via navigate's state (call navigate(`/visualizer/${newId}`, { state: { image:
base64Image, id: newId } }) and then read state?.image in visualizer.$id.tsx via
useLocation) or save it to a shared store/context or localStorage under the
newId (e.g., setImageForId(newId, base64Image)) and then navigate to
`/visualizer/${newId}` so visualizer.$id.tsx can retrieve the image by id;
ensure the symbol handleUploadComplete is modified and visualizer.$id.tsx reads
the same storage mechanism.
In `@app/routes/visualizer`.$id.tsx:
- Around line 3-9: The VisualizerId component is a placeholder and currently has
no access to the uploaded base64 image because handleUploadComplete in
app/routes/home.tsx discards the base64Image and you never extract the route
:id; fix by persisting or passing the image before navigation and reading it in
VisualizerId: either (A) store base64Image in shared state (Context/Zustand) in
handleUploadComplete and have VisualizerId read it, (B) upload/persist the image
to backend in handleUploadComplete and fetch it in VisualizerId using the id
param, or (C) pass the image via navigate(`/visualizer/${newId}`, { state: {
image: base64Image } }) and then read it from location.state in VisualizerId;
additionally import and use useParams (from react-router) inside VisualizerId to
extract the id param when you need to fetch by id.
In `@components/Upload.tsx`:
- Around line 66-73: handleChange currently skips file-type validation while
handleDrop enforces image-only via droppedFile.type.startsWith('image/'); update
handleChange to mirror that check: after obtaining selectedFile in handleChange,
ensure isSignedIn and that selectedFile.type.startsWith('image/') before calling
processFile(selectedFile), and reject or ignore non-image files (optionally
trigger the same user feedback/path used elsewhere). This keeps validation
consistent between handleDrop and handleChange and relies on the same
processFile/isSignedIn flow.
- Around line 23-41: Add a FileReader error handler: define reader.onerror
alongside reader.onloadend to clear any running interval, reset UI progress via
setProgress(0) (or another safe fallback), and notify the parent via an error
callback (e.g., call onError?.(event) if you add that prop) or at minimum log
the error (console.error) so failures aren't silent; update the code around
FileReader, reader.onloadend, reader.readAsDataURL(file), PROGRESS_INCREMENT,
PROGRESS_INTERVAL_MS and REDIRECT_DELAY_MS to ensure the interval is cleared and
no onComplete is called when an error occurs.
- Line 101: The UI claims a 50 MB max but no enforcement exists; update
processFile (and callers handleDrop and handleChange) to check file.size and
reject files > 50 * 1024 * 1024 before any base64 conversion or heavy
processing, returning/throwing or calling the existing error handler to surface
a user-friendly message and abort further processing; ensure processFile is the
single place that enforces the limit so handleDrop/handleChange can quickly
delegate and skip reads when the check fails.
- Around line 27-39: The interval created in the Upload progress logic
(setInterval that calls setProgress using PROGRESS_INCREMENT and
PROGRESS_INTERVAL_MS and later triggers onComplete(base64Data) after
REDIRECT_DELAY_MS) isn't cleared on component unmount; store the interval id
(and any timeout id) in a ref or ensure the interval is created inside a
useEffect and return a cleanup that calls clearInterval(intervalId) and
clearTimeout(timeoutId) to prevent state updates after unmount and eliminate the
memory leak/warnings from setProgress/onComplete being called on an unmounted
component.
---
Nitpick comments:
In `@app/routes/home.tsx`:
- Around line 63-65: Remove the commented-out debug snippet that references the
Upload component and its onComplete handler (the block with Upload, onComplete
and console.log(base64Data)); delete this commented code entirely from
app/routes/home.tsx to keep the file clean and avoid leftover debug examples.
- Around line 83-85: The <img> tag in app/routes/home.tsx uses a hardcoded
external src which can break; change it to use a local asset or an
environment-driven base URL: replace the literal src string on the <img> element
with either an imported/local asset (e.g., import or new URL(...) for the
preview image) or construct the src from an env var like PREVIEW_BASE_URL +
"/projects/1770803585402/rendered.png" and fall back to a bundled local
placeholder; update the <img> element reference accordingly to use the new
variable instead of the hardcoded URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ac19852-ec56-4473-aee7-cac16da2c841
📒 Files selected for processing (5)
app/routes.tsapp/routes/home.tsxapp/routes/visualizer.$id.tsxcomponents/Upload.tsxlib/constants.ts
Comment on lines
+19
to
+25
| const handleUploadComplete = async (base64Image: string) => { | ||
| const newId = Date.now().toString(); | ||
|
|
||
| navigate(`/visualizer/${newId}`); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Critical: Uploaded image data is discarded.
handleUploadComplete receives base64Image containing the full floor plan data but never stores it. After navigating to /visualizer/${newId}, the image data is lost and the visualizer page has no way to retrieve it.
This breaks the core upload-to-visualize flow.
🔧 Option 1: Pass via route state
const handleUploadComplete = async (base64Image: string) => {
const newId = Date.now().toString();
- navigate(`/visualizer/${newId}`);
- return true;
+ navigate(`/visualizer/${newId}`, { state: { image: base64Image } });
}Then in visualizer.$id.tsx:
import { useLocation } from 'react-router';
const { state } = useLocation();
const image = state?.image;🔧 Option 2: Store in context/global state before navigating
+import { useUploadStore } from 'stores/upload'; // or context
+
const handleUploadComplete = async (base64Image: string) => {
const newId = Date.now().toString();
+ useUploadStore.getState().setImage(newId, base64Image);
navigate(`/visualizer/${newId}`);
- return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleUploadComplete = async (base64Image: string) => { | |
| const newId = Date.now().toString(); | |
| navigate(`/visualizer/${newId}`); | |
| return true; | |
| } | |
| const handleUploadComplete = async (base64Image: string) => { | |
| const newId = Date.now().toString(); | |
| navigate(`/visualizer/${newId}`, { state: { image: base64Image } }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/routes/home.tsx` around lines 19 - 25, handleUploadComplete is discarding
the base64Image (the uploaded floor plan) before navigation, so the visualizer
page cannot access it; update handleUploadComplete to persist the image before
navigating—either pass it via navigate's state (call
navigate(`/visualizer/${newId}`, { state: { image: base64Image, id: newId } })
and then read state?.image in visualizer.$id.tsx via useLocation) or save it to
a shared store/context or localStorage under the newId (e.g.,
setImageForId(newId, base64Image)) and then navigate to `/visualizer/${newId}`
so visualizer.$id.tsx can retrieve the image by id; ensure the symbol
handleUploadComplete is modified and visualizer.$id.tsx reads the same storage
mechanism.
Comment on lines
+3
to
+9
| const VisualizerId = () => { | ||
| return ( | ||
| <div> | ||
| VisualizerId | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Placeholder component lacks access to uploaded image data.
This route receives an :id parameter but the uploaded image (base64 data) is never persisted before navigation. The handleUploadComplete in app/routes/home.tsx (lines 19-25) discards the base64Image argument, so this component has no mechanism to retrieve the actual floor plan image.
Consider one of:
- Store the image in a shared state manager (context, Zustand, etc.) before navigating
- Persist to backend/storage and fetch here using the
id - Pass via route state:
navigate(\/visualizer/${newId}`, { state: { image: base64Image } })`
Also, the :id param is not being extracted — you'll need useParams from react-router to access it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/routes/visualizer`.$id.tsx around lines 3 - 9, The VisualizerId component
is a placeholder and currently has no access to the uploaded base64 image
because handleUploadComplete in app/routes/home.tsx discards the base64Image and
you never extract the route :id; fix by persisting or passing the image before
navigation and reading it in VisualizerId: either (A) store base64Image in
shared state (Context/Zustand) in handleUploadComplete and have VisualizerId
read it, (B) upload/persist the image to backend in handleUploadComplete and
fetch it in VisualizerId using the id param, or (C) pass the image via
navigate(`/visualizer/${newId}`, { state: { image: base64Image } }) and then
read it from location.state in VisualizerId; additionally import and use
useParams (from react-router) inside VisualizerId to extract the id param when
you need to fetch by id.
Comment on lines
+23
to
+41
| const reader = new FileReader(); | ||
| reader.onloadend = () => { | ||
| const base64Data = reader.result as string; | ||
|
|
||
| const interval = setInterval(() => { | ||
| setProgress((prev) => { | ||
| const next = prev + PROGRESS_INCREMENT; | ||
| if (next >= 100) { | ||
| clearInterval(interval); | ||
| setTimeout(() => { | ||
| onComplete?.(base64Data); | ||
| }, REDIRECT_DELAY_MS); | ||
| return 100; | ||
| } | ||
| return next; | ||
| }); | ||
| }, PROGRESS_INTERVAL_MS); | ||
| }; | ||
| reader.readAsDataURL(file); |
There was a problem hiding this comment.
Missing FileReader error handling.
If FileReader fails (e.g., file is corrupted or read permission denied), there's no onerror handler. The component would silently fail, leaving the user with no feedback.
🔧 Proposed fix
const reader = new FileReader();
+reader.onerror = () => {
+ setFile(null);
+ setProgress(0);
+ // Optionally show error state to user
+};
reader.onloadend = () => {
...
};
reader.readAsDataURL(file);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const reader = new FileReader(); | |
| reader.onloadend = () => { | |
| const base64Data = reader.result as string; | |
| const interval = setInterval(() => { | |
| setProgress((prev) => { | |
| const next = prev + PROGRESS_INCREMENT; | |
| if (next >= 100) { | |
| clearInterval(interval); | |
| setTimeout(() => { | |
| onComplete?.(base64Data); | |
| }, REDIRECT_DELAY_MS); | |
| return 100; | |
| } | |
| return next; | |
| }); | |
| }, PROGRESS_INTERVAL_MS); | |
| }; | |
| reader.readAsDataURL(file); | |
| const reader = new FileReader(); | |
| reader.onerror = () => { | |
| setFile(null); | |
| setProgress(0); | |
| // Optionally show error state to user | |
| }; | |
| reader.onloadend = () => { | |
| const base64Data = reader.result as string; | |
| const interval = setInterval(() => { | |
| setProgress((prev) => { | |
| const next = prev + PROGRESS_INCREMENT; | |
| if (next >= 100) { | |
| clearInterval(interval); | |
| setTimeout(() => { | |
| onComplete?.(base64Data); | |
| }, REDIRECT_DELAY_MS); | |
| return 100; | |
| } | |
| return next; | |
| }); | |
| }, PROGRESS_INTERVAL_MS); | |
| }; | |
| reader.readAsDataURL(file); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/Upload.tsx` around lines 23 - 41, Add a FileReader error handler:
define reader.onerror alongside reader.onloadend to clear any running interval,
reset UI progress via setProgress(0) (or another safe fallback), and notify the
parent via an error callback (e.g., call onError?.(event) if you add that prop)
or at minimum log the error (console.error) so failures aren't silent; update
the code around FileReader, reader.onloadend, reader.readAsDataURL(file),
PROGRESS_INCREMENT, PROGRESS_INTERVAL_MS and REDIRECT_DELAY_MS to ensure the
interval is cleared and no onComplete is called when an error occurs.
Comment on lines
+27
to
+39
| const interval = setInterval(() => { | ||
| setProgress((prev) => { | ||
| const next = prev + PROGRESS_INCREMENT; | ||
| if (next >= 100) { | ||
| clearInterval(interval); | ||
| setTimeout(() => { | ||
| onComplete?.(base64Data); | ||
| }, REDIRECT_DELAY_MS); | ||
| return 100; | ||
| } | ||
| return next; | ||
| }); | ||
| }, PROGRESS_INTERVAL_MS); |
There was a problem hiding this comment.
Interval not cleaned up on unmount — potential memory leak.
If the component unmounts while the progress animation is running (e.g., user navigates away), the interval continues and attempts to update state on an unmounted component. This can cause memory leaks and React warnings.
🛠️ Suggested fix using useEffect cleanup or useRef
+import React, {useCallback, useState, useRef, useEffect} from 'react'
...
const Upload = ({ onComplete }: UploadProps) => {
const [file, setFile] = useState<File | null>(null);
const [isDragging, setIsDragging] = useState(false);
const [progress, setProgress] = useState(0);
+ const intervalRef = useRef<ReturnType<typeof setInterval> | null>(null);
const { isSignedIn } = useOutletContext<AuthContext>();
+ useEffect(() => {
+ return () => {
+ if (intervalRef.current) {
+ clearInterval(intervalRef.current);
+ }
+ };
+ }, []);
+
const processFile = useCallback((file: File) => {
...
reader.onloadend = () => {
const base64Data = reader.result as string;
- const interval = setInterval(() => {
+ intervalRef.current = setInterval(() => {
setProgress((prev) => {
const next = prev + PROGRESS_INCREMENT;
if (next >= 100) {
- clearInterval(interval);
+ clearInterval(intervalRef.current!);
+ intervalRef.current = null;
...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/Upload.tsx` around lines 27 - 39, The interval created in the
Upload progress logic (setInterval that calls setProgress using
PROGRESS_INCREMENT and PROGRESS_INTERVAL_MS and later triggers
onComplete(base64Data) after REDIRECT_DELAY_MS) isn't cleared on component
unmount; store the interval id (and any timeout id) in a ref or ensure the
interval is created inside a useEffect and return a cleanup that calls
clearInterval(intervalId) and clearTimeout(timeoutId) to prevent state updates
after unmount and eliminate the memory leak/warnings from setProgress/onComplete
being called on an unmounted component.
Comment on lines
+66
to
+73
| const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| if (!isSignedIn) return; | ||
|
|
||
| const selectedFile = e.target.files?.[0]; | ||
| if (selectedFile) { | ||
| processFile(selectedFile); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Inconsistent file type validation between handlers.
handleDrop validates droppedFile.type.startsWith('image/') (line 61), but handleChange accepts any file without type checking. This could allow non-image files via the file picker despite the accept attribute (which is only a hint and can be bypassed).
🔧 Proposed fix
const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => {
if (!isSignedIn) return;
const selectedFile = e.target.files?.[0];
- if (selectedFile) {
+ if (selectedFile && selectedFile.type.startsWith('image/')) {
processFile(selectedFile);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| if (!isSignedIn) return; | |
| const selectedFile = e.target.files?.[0]; | |
| if (selectedFile) { | |
| processFile(selectedFile); | |
| } | |
| }; | |
| const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| if (!isSignedIn) return; | |
| const selectedFile = e.target.files?.[0]; | |
| if (selectedFile && selectedFile.type.startsWith('image/')) { | |
| processFile(selectedFile); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/Upload.tsx` around lines 66 - 73, handleChange currently skips
file-type validation while handleDrop enforces image-only via
droppedFile.type.startsWith('image/'); update handleChange to mirror that check:
after obtaining selectedFile in handleChange, ensure isSignedIn and that
selectedFile.type.startsWith('image/') before calling processFile(selectedFile),
and reject or ignore non-image files (optionally trigger the same user
feedback/path used elsewhere). This keeps validation consistent between
handleDrop and handleChange and relies on the same processFile/isSignedIn flow.
| "Click to upload or just drag and drop" | ||
| ): ("Sign in or sign up with Puter to upload")} | ||
| </p> | ||
| <p className="help">Maximum file size 50 MB.</p> |
There was a problem hiding this comment.
File size limit stated in UI is not enforced.
The UI states "Maximum file size 50 MB" but no validation exists in processFile, handleDrop, or handleChange. Large files could cause browser memory issues when converted to base64.
🔧 Proposed fix — add size check in processFile
+const MAX_FILE_SIZE_MB = 50;
+const MAX_FILE_SIZE_BYTES = MAX_FILE_SIZE_MB * 1024 * 1024;
+
const processFile = useCallback((file: File) => {
if (!isSignedIn) return;
+ if (file.size > MAX_FILE_SIZE_BYTES) {
+ // Show error to user
+ return;
+ }
...
}, [isSignedIn, onComplete]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/Upload.tsx` at line 101, The UI claims a 50 MB max but no
enforcement exists; update processFile (and callers handleDrop and handleChange)
to check file.size and reject files > 50 * 1024 * 1024 before any base64
conversion or heavy processing, returning/throwing or calling the existing error
handler to surface a user-friendly message and abort further processing; ensure
processFile is the single place that enforces the limit so
handleDrop/handleChange can quickly delegate and skip reads when the check
fails.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
components/Upload.tsx (3)
1-1:⚠️ Potential issue | 🟠 MajorTimers are not cleaned up on unmount (interval + redirect timeout).
The interval and timeout created in this flow are never tracked/cleared if the component unmounts, which can trigger state updates after unmount.
Suggested fix
-import React, {useCallback, useState} from 'react' +import React, {useCallback, useEffect, useRef, useState} from 'react' const Upload = ({ onComplete }: UploadProps) => { + const intervalRef = useRef<ReturnType<typeof setInterval> | null>(null); + const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); + + useEffect(() => { + return () => { + if (intervalRef.current) clearInterval(intervalRef.current); + if (timeoutRef.current) clearTimeout(timeoutRef.current); + }; + }, []); + const processFile = useCallback((file: File) => { + if (intervalRef.current) clearInterval(intervalRef.current); + if (timeoutRef.current) clearTimeout(timeoutRef.current); ... - const interval = setInterval(() => { + intervalRef.current = setInterval(() => { setProgress((prev) => { const next = prev + PROGRESS_INCREMENT; if (next >= 100) { - clearInterval(interval); - setTimeout(() => { + if (intervalRef.current) clearInterval(intervalRef.current); + timeoutRef.current = setTimeout(() => { onComplete?.(base64Data); }, REDIRECT_DELAY_MS); return 100; } return next; }); }, PROGRESS_INTERVAL_MS);Also applies to: 31-38, 46-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Upload.tsx` at line 1, The Upload component creates an interval and a redirect timeout that are never cleared on unmount; add refs (e.g., intervalRef and timeoutRef via React.useRef<number | null>) to store the IDs returned by setInterval/setTimeout, clear any existing timers before assigning new ones, and add a useEffect cleanup that calls clearInterval(intervalRef.current) and clearTimeout(timeoutRef.current) (and nulls the refs); ensure you also clear the timers immediately before performing the redirect or any state updates to avoid updates-after-unmount.
17-22:⚠️ Potential issue | 🟠 MajorValidation is inconsistent and does not enforce the stated 50 MB limit.
Line 106 promises a 50 MB cap, but there’s no size enforcement. Also,
handleChangeaccepts non-image files whilehandleDropfilters types. Move both checks intoprocessFileso both entry points behave identically.Suggested fix
+const ALLOWED_TYPES = ['image/jpeg', 'image/png'] as const; +const MAX_FILE_SIZE_BYTES = 50 * 1024 * 1024; + const processFile = useCallback((file: File) => { if (!isSignedIn) return; + if (!ALLOWED_TYPES.includes(file.type as (typeof ALLOWED_TYPES)[number])) return; + if (file.size > MAX_FILE_SIZE_BYTES) return; setFile(file); setProgress(0); ... }, [isSignedIn, onComplete]); const handleDrop = (e: React.DragEvent) => { ... - const droppedFile = e.dataTransfer.files[0]; - const allowedTypes = ['image/jpeg','image/png']; - if (droppedFile && allowedTypes.includes(droppedFile.type)) { - processFile(droppedFile); - } + const droppedFile = e.dataTransfer.files[0]; + if (droppedFile) processFile(droppedFile); }; const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { ... const selectedFile = e.target.files?.[0]; - if (selectedFile) { - processFile(selectedFile); - } + if (selectedFile) processFile(selectedFile); };Also applies to: 64-67, 74-77, 106-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Upload.tsx` around lines 17 - 22, processFile currently only checks isSignedIn and doesn't enforce the 50 MB size limit or image MIME-types, causing inconsistent behavior between handleChange and handleDrop; move the file type and size validation into processFile (check file.type startsWith('image/') and file.size <= 50 * 1024 * 1024) and make both handleChange and handleDrop delegate to processFile so both entry points share identical validation, setting the appropriate error state (e.g., setError or similar) and early-return on invalid files.
24-30:⚠️ Potential issue | 🟠 Major
onloadendcan still complete the upload after a read error.Line 28 uses
reader.onloadend, which runs even on failure. That means the error path can still schedule redirect and callonCompletewith invalid data.Suggested fix
- reader.onerror = () =>{ - setFile(null); - setProgress(0); - } - reader.onloadend = () => { - const base64Data = reader.result as string; + reader.onerror = () => { + setFile(null); + setProgress(0); + }; + reader.onload = () => { + if (typeof reader.result !== "string") { + setFile(null); + setProgress(0); + return; + } + const base64Data = reader.result;Also applies to: 36-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Upload.tsx` around lines 24 - 30, reader.onloadend is running even after a read error so the success path can run with invalid data; modify the upload completion logic (the reader.onloadend handler and the similar block at lines 36-38) to first check for reader.error (or verify reader.result is a non-empty string) and only proceed to call onComplete/redirect/setFile when there is no error; alternatively move the success logic into reader.onload and keep reader.onerror to handle failures (calling setFile(null) and setProgress(0) and preventing further actions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/Upload.tsx`:
- Line 1: The Upload component creates an interval and a redirect timeout that
are never cleared on unmount; add refs (e.g., intervalRef and timeoutRef via
React.useRef<number | null>) to store the IDs returned by
setInterval/setTimeout, clear any existing timers before assigning new ones, and
add a useEffect cleanup that calls clearInterval(intervalRef.current) and
clearTimeout(timeoutRef.current) (and nulls the refs); ensure you also clear the
timers immediately before performing the redirect or any state updates to avoid
updates-after-unmount.
- Around line 17-22: processFile currently only checks isSignedIn and doesn't
enforce the 50 MB size limit or image MIME-types, causing inconsistent behavior
between handleChange and handleDrop; move the file type and size validation into
processFile (check file.type startsWith('image/') and file.size <= 50 * 1024 *
1024) and make both handleChange and handleDrop delegate to processFile so both
entry points share identical validation, setting the appropriate error state
(e.g., setError or similar) and early-return on invalid files.
- Around line 24-30: reader.onloadend is running even after a read error so the
success path can run with invalid data; modify the upload completion logic (the
reader.onloadend handler and the similar block at lines 36-38) to first check
for reader.error (or verify reader.result is a non-empty string) and only
proceed to call onComplete/redirect/setFile when there is no error;
alternatively move the success logic into reader.onload and keep reader.onerror
to handle failures (calling setFile(null) and setProgress(0) and preventing
further actions).
This was referenced Mar 26, 2026
Merged
Merged
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
1 participant
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.
Summary by CodeRabbit