Skip to content

feat: upload indicator for images#369

Open
justinedagreat wants to merge 2 commits intomasterfrom
origin/vps-12-show-upload-progress-indicator-for-resources
Open

feat: upload indicator for images#369
justinedagreat wants to merge 2 commits intomasterfrom
origin/vps-12-show-upload-progress-indicator-for-resources

Conversation

@justinedagreat
Copy link
Copy Markdown
Contributor

@justinedagreat justinedagreat commented Apr 30, 2026

Issue

To let the user know that an image is being uploaded, it needs a clear indication that something is happening. This prevents users from being misled into thinking nothing is happening and from uploading duplicates.

Solution

Introduced a LoadingOverlay function that blurs the canvas and displays a loading animation while the image is being uploaded.

  • Introduced loading states to represent that something is being uploaded. The default value is false, since it will only change to' true' when the user tries to upload.
  • When user is uploading an image, the addNewImage function is being called, and it will then set loading=true. Then the actual upload process begins.
  • After the upload process is finished, it will then set the loading state to false

Risk

  • The entire canvas is locked during upload, cannot continue editing and cannot cancel when the upload is too slow.
  • It could feel like it is frozen when the image is taking time to upload.

Checklist:

  • Acceptance criteria met
  • Wiki documentation is written and up to date
  • Unit tests written and passing
  • Integration tests written and passing
  • Continuous integration build passing

@linear
Copy link
Copy Markdown

linear Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

@del-ereno del-ereno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works great!
could add a toast notif if the image upload fails w the finally, but that might not fall under this ticket

@hazikchaudhry
Copy link
Copy Markdown
Contributor

🛸 Orbit CodeReview

PR Summary

The PR adds a LoadingOverlay component to indicate when an image is being uploaded. It introduces a loading state managed in the editor store, toggles it on upload start, and resets it after the upload finishes. The canvas is dimmed with pointer-events-none while loading.

Changes Overview

File Changes Vibe
frontend/src/features/authoring/canvas/Canvas.tsx Import LoadingOverlay, read loading from store, add <LoadingOverlay /> and conditional pointer-events-none class 🟢 clean
frontend/src/features/authoring/canvas/LoadingOverlay.tsx New component that renders a full‑screen overlay with a spinner when loading is true 🟢 clean
frontend/src/features/authoring/images.tsx Use useEditorStore to call setLoading(true/false) around Firebase upload logic 🟡 needs work
frontend/src/features/authoring/stores/editor.ts Add loading field and setLoading setter to the EditorState interface and store initialization 🟢 clean

What's Good

  • Clear separation of concerns: loading UI is isolated in its own LoadingOverlay component.
  • The loading state is persisted in the editor store, making it globally accessible.
  • The overlay uses a nice blurred backdrop and spinner for visual feedback.
  • Error handling guarantees setLoading(false) runs via finally, preventing stuck loading states.

Issues

Severity File Issue
🔴 High frontend/src/features/authoring/canvas/Canvas.tsx The canvas receives pointer-events-none while loading=true, completely locking user interaction (no cancel, no pointer capture). This can make the editor feel frozen, especially on slow uploads.
🟡 Medium frontend/src/features/authoring/canvas/Canvas.tsx Conditional class generation (${loading ? "pointer-events-none" : ""}) is a bit verbose; could be extracted to a utility or enum for readability.
🟡 Medium frontend/src/features/authoring/images.tsx The upload logic still contains the original Firebase auth flow; if the user navigates away or the component unmounts before the upload finishes, there could be memory leaks or state updates on unmounted components.
🟢 Low frontend/src/features/authoring/canvas/Canvas.tsx Direct import of LoadingOverlay from ./LoadingOverlay.tsx is fine, but the file is placed under canvas/ – consider moving generic UI components to a shared ui/ folder for reusability.
🟡 Medium frontend/src/features/authoring/stores/editor.ts The setLoading setter is added to the mutable EditorState interface, but the type definition mixes UI concerns (loading) with business state. Could be renamed to isUploading for clarity.

Suggestions

  1. Cancellation Mechanism – Add an “Cancel Upload” button or a timeout that aborts the Firebase uploadBytes request. This would let users recover from a stalled upload instead of being forced to wait.
    // In Canvas.tsx
    const [uploadCancel, setUploadCancel] = useState(false);
    useEffect(() => {
      if (uploadCancel) {
        // Abort ongoing upload (e.g., cancel token or controller)
      }
    }, [uploadCancel]);
  2. Progress Indicator – Instead of a simple spinner, consider showing a progress bar or percentage to give users a sense of how long the upload might take.
  3. Extract Loading Logic – Move the pointer-events-none handling into a small helper component or CSS class name generator to keep JSX tidy.
  4. Unit Tests – Add tests that verify:
    • LoadingOverlay renders only when loading is true.
    • setLoading(true) is called before the upload promise resolves.
    • setLoading(false) is called in the finally block even when an error occurs.
  5. Abort Handling – Guard against state updates after component unmount by checking a mounted flag or using React’s useIsMounted pattern.
    const isMounted = useRef(true);
    useEffect(() => () => { isMounted.current = false; }, []);
    // In finally: if (isMounted.current) setLoading(false);
  6. Naming – Rename loading to isUploading (or similar) throughout the store to better reflect that it specifically tracks image uploads, not generic loading.

Overall Score

Category Score
Code Quality 7.5/10
Readability 8/10
Test Coverage 5/10 (needs more)
Overall 7/10

🎋 Orbit's Farewell Haiku

Canvas dims, spinner spins,
Uploads linger, users wonder—
Cancel the storm.

Copy link
Copy Markdown
Contributor

@harbassan harbassan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, just this one thing

Comment on lines +4 to +7
const loading = useEditorStore((state) => state.loading);

if (!loading) return null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this overlay component should just be the jsx, so that it can be general purpose. The conditional rendering of it should be handled in the parent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants