Image upload - #25
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive image upload feature for the DocVerse platform. The changes enable users to upload images directly within the markdown editor and manage them through a gallery interface.
- Image upload functionality with database storage and tagging
- Enhanced markdown editor with custom image upload command
- Database migration system and new tables for file storage
- Tag restrictions for admin-only tags like "challenges"
Reviewed Changes
Copilot reviewed 25 out of 30 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/types/index.ts | Added Challenge interface and new post status types |
| app/src/services/api-client.ts | Added file upload endpoints and challenge management |
| app/src/pages/create-post.tsx | Integrated image upload component with markdown editor |
| app/src/components/ui/* | New UI components for image upload, gallery, and tagging |
| apis/src/routers/files.py | New file upload router with authentication and tagging |
| apis/src/services/database/* | Database migration system and file storage operations |
| apis/bootstrap.sql | Added content_uploads and site_settings tables |
Files not reviewed (1)
- app/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
app/src/hooks/use-challenges.ts:1
- Use strict equality (===) instead of loose equality (==) for comparison to avoid type coercion issues.
import { useState, useMemo, useCallback, useEffect } from 'react';
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Set up global callback for MDEditor image insertion | ||
| useEffect(() => { | ||
| console.log('Setting up global callbacks...'); | ||
| window.insertMarkdownCallback = handleImageInsert; | ||
| window.showImageUploadDialog = () => { | ||
| console.log('showImageUploadDialog called!'); | ||
| setShowImageDialog(true); | ||
| }; | ||
|
|
||
| return () => { | ||
| delete window.insertMarkdownCallback; | ||
| delete window.showImageUploadDialog; | ||
| }; | ||
| }, []); | ||
|
|
||
| // Create custom commands for MDEditor | ||
| const imageUploadCommand = createImageUploadCommand(); |
There was a problem hiding this comment.
Adding functions to the global window object is an anti-pattern that can lead to memory leaks and naming conflicts. Consider using React refs or a more React-friendly communication pattern between the markdown editor and the upload component.
| // Set up global callback for MDEditor image insertion | |
| useEffect(() => { | |
| console.log('Setting up global callbacks...'); | |
| window.insertMarkdownCallback = handleImageInsert; | |
| window.showImageUploadDialog = () => { | |
| console.log('showImageUploadDialog called!'); | |
| setShowImageDialog(true); | |
| }; | |
| return () => { | |
| delete window.insertMarkdownCallback; | |
| delete window.showImageUploadDialog; | |
| }; | |
| }, []); | |
| // Create custom commands for MDEditor | |
| const imageUploadCommand = createImageUploadCommand(); | |
| // Use React state and props for dialog and markdown insertion | |
| // Remove global window callback setup/cleanup. | |
| // Create custom commands for MDEditor | |
| const imageUploadCommand = createImageUploadCommand({ | |
| onShowDialog: () => setShowImageDialog(true), | |
| onInsertMarkdown: handleImageInsert, | |
| }); |
| logger.info(f"👤 Current user: {current_user}") | ||
|
|
||
| # Check if current_user has required fields | ||
| if 'user_id' not in current_user: | ||
| logger.error(f"❌ Current user missing 'user_id' field: {current_user}") |
There was a problem hiding this comment.
Logging the entire current_user object may expose sensitive user information in logs. Consider logging only the user ID or a sanitized version of the user data.
| logger.info(f"👤 Current user: {current_user}") | |
| # Check if current_user has required fields | |
| if 'user_id' not in current_user: | |
| logger.error(f"❌ Current user missing 'user_id' field: {current_user}") | |
| logger.info(f"👤 Current user ID: {current_user.get('user_id', 'unknown')}") | |
| # Check if current_user has required fields | |
| if 'user_id' not in current_user: | |
| logger.error(f"❌ Current user missing 'user_id' field. User ID: {current_user.get('user_id', 'unknown')}") |
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Cache configuration | ||
| CACHE_DIR = Path("/tmp/docverse_files") |
There was a problem hiding this comment.
Hard-coded cache directory path should be configurable through environment variables or settings to support different deployment environments and avoid potential permission issues.
No description provided.