Skip to content

web : fix file upload uuid.extension format#19

Open
surajmn1 wants to merge 4 commits intomainfrom
fix/web/uuid
Open

web : fix file upload uuid.extension format#19
surajmn1 wants to merge 4 commits intomainfrom
fix/web/uuid

Conversation

@surajmn1
Copy link
Copy Markdown
Contributor

@surajmn1 surajmn1 commented Nov 6, 2025

Summary by CodeRabbit

  • New Features

    • Automatic file-name sanitization on upload with unique UUID-based names while preserving the original name in metadata.
  • Refactor

    • Chat session initialization tightened to require proper authentication/configuration before creating new chats.
  • User-facing Data Changes

    • Project metadata and date/user fields now use a different naming convention—date and author displays updated accordingly (may affect integrations).

@claude
Copy link
Copy Markdown

claude Bot commented Nov 6, 2025

Claude Code is working…

I'll analyze this and get back to you.

View job run

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 6, 2025

Walkthrough

The PR renames Project interface fields to snake_case, updates UI components to use those fields, adds a UUID-based filename sanitization utility and integrates it into the Uppy uploader (preserving originalName in meta), and makes chat session creation conditional on authentication/org presence.

Changes

Cohort / File(s) Change Summary
API Schema Update
web/src/lib/api-client.ts
Renamed Project interface properties: createdAtcreated_at, updatedAtupdated_at, createdBycreated_by, updatedByupdated_by.
Project Component Updates
web/src/components/project/project-card.tsx, web/src/components/project/inline-project-editor.tsx
Updated UI usage to read snake_case project fields (created_at, updated_at, created_by) instead of camelCase equivalents.
File Upload & Sanitization
web/src/lib/utils/sanitize-filename.ts, web/src/components/dataset/unified-uploader.tsx
Added sanitizeAndUniquifyFilename() (UUID-based, preserves extension). Integrated into Uppy via onBeforeFileAdded to replace file.name with sanitized name and store original in meta.originalName.
Chat Session Auth Logic
web/src/hooks/use-chat-session.ts
Made new-chat creation require either auth enabled or presence of organizationId from useAuthStore; added related imports and effect dependencies.
Package Dependency
web/package.json
Added uuid dependency (^13.0.0).

Sequence Diagram(s)

sequenceDiagram
  participant U as Uppy (uploader)
  participant User
  participant Util as sanitizeAndUniquifyFilename
  User->>U: add file (originalName.ext)
  alt onBeforeFileAdded hook
    U->>Util: sanitizeAndUniquifyFilename(originalName.ext)
    Util-->>U: uuid.ext
    U->>U: set file.name = uuid.ext\nset meta.originalName = originalName.ext
  end
  U->>U: continue processing / upload
Loading
sequenceDiagram
  participant Hook as useChatSession
  participant Auth as useAuthStore
  participant App

  App->>Hook: mount / contexts selected
  Hook->>Auth: read organizationId, NEXT_PUBLIC_ENABLE_AUTH
  alt auth enabled OR organizationId present
    Hook->>Hook: create new chat (if other conditions met)
  else
    Hook-->>App: skip auto-create new chat
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Areas requiring extra attention:
    • All usages of the Project interface across the codebase (ensure no remaining camelCase references).
    • Potential runtime / typing mismatches where API returns camelCase but types expect snake_case (or vice versa).
    • Uppy integration: confirm metadata propagation, file object mutation behavior, and that sanitization preserves extensions and downstream handling.
    • Chat initialization edge cases when auth flags or organizationId change.

Suggested reviewers

  • Nirlep5252

Poem

🐰 I nudge names into neat, snaky rows,

UUID seeds where old filenames go,
Chats wait for keys or orgs to bloom,
Cards read dates from their new little room,
A hop, a patch, and off I zoom! 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'web : fix file upload uuid.extension format' accurately captures the main change: implementing UUID-based filename sanitization with preserved extensions in the file upload component.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/web/uuid

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4801b6d and 62d56d6.

⛔ Files ignored due to path filters (1)
  • web/bun.lockb is excluded by !**/bun.lockb
📒 Files selected for processing (1)
  • web/package.json (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Analyze (go)
  • GitHub Check: claude-review
🔇 Additional comments (1)
web/package.json (1)

94-94: ✅ Dependency addition looks good.

Version 13.0.0 is the latest version of the uuid package, and the caret constraint ^13.0.0 is appropriate. The uuid package is widely used (71,402 projects) and has no dependencies, making it a solid choice for generating UUIDs in the filename sanitization utility. The placement in the runtime dependencies section is correct.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link
Copy Markdown

claude Bot commented Nov 6, 2025

Claude finished @surajmn1's task —— View job


Comprehensive Code Review

I've analyzed all the changed files in this PR. Here's my detailed feedback:

Todo List:

  • Read and analyze all changed files
  • Review API schema changes (camelCase to snake_case)
  • Analyze filename sanitization utility for security
  • Check authentication logic changes
  • Evaluate code quality and best practices
  • Look for potential bugs and consistency issues
  • Assess performance and security concerns
  • Check test coverage implications
  • Provide comprehensive feedback

🟢 Positive Aspects

1. API Schema Consistency

  • ✅ Successfully migrated Project interface from camelCase to snake_case
  • ✅ Updated all relevant components (project-card.tsx, inline-project-editor.tsx) to use new properties
  • ✅ Maintains consistency with backend API expectations

2. Filename Sanitization Implementation

  • ✅ Good security practice using UUID-based filenames to prevent collisions
  • ✅ Preserves original file extensions properly
  • ✅ Stores original filename in metadata for reference
  • ✅ Clear documentation with JSDoc comments

🟡 Issues & Recommendations

1. Critical: Incomplete API Migration (High Priority)

Found inconsistencies in the codebase migration:

In /src/components/chat/chat-view.tsx:87-93:

// Still using old camelCase properties
typeof (message as any).createdAt === "string"
? (message as any).createdAt
: (message as any).createdAt instanceof Date
? (message as any).createdAt.toISOString()

Recommendation: Search for ALL instances of createdAt, updatedAt, createdBy, updatedBy and update them to snake_case variants. This could cause runtime errors if the API only returns snake_case properties.

2. Authentication Logic Concerns (Medium Priority)

In use-chat-session.ts:72:

isNewChat && (isAuthDisabled || organizationId) && !selectedChatId

Issues:

  • The condition (isAuthDisabled || organizationId) could create unexpected behavior
  • When auth is disabled, organizationId might be undefined, but the condition would still pass
  • When auth is enabled but organizationId is null/undefined, chat creation would be blocked

Recommendation:

// More explicit condition
isNewChat && 
(isAuthDisabled || (organizationId != null)) && 
!selectedChatId

3. Security Considerations (Medium Priority)

Filename Sanitization:

  • ✅ UUID prevents directory traversal attacks
  • ✅ Extension preservation is safe
  • ⚠️ Missing validation: No validation of file extensions against allowed types
  • ⚠️ Missing length check: Extension could theoretically be very long

Recommendation:

export function sanitizeAndUniquifyFilename(originalName: string): string {
    const uniqueId = uuidv4();
    const extensionMatch = originalName.match(/\.[^.]{1,10}$/); // Limit extension length
    const extension = extensionMatch ? extensionMatch[0] : '';
    
    // Optional: Validate against allowed extensions
    const allowedExtensions = ['.csv', '.json', '.xlsx', '.parquet'];
    if (extension && !allowedExtensions.includes(extension.toLowerCase())) {
        return `${uniqueId}.unknown`; // or throw error
    }
    
    return `${uniqueId}${extension}`;
}

4. Type Safety Issues (Medium Priority)

In unified-uploader.tsx:

  • Using any types in several places (lines 48, 253, 262)
  • Type assertions without proper validation

Recommendation: Define proper TypeScript interfaces for Uppy plugin types instead of using any.

5. Performance Considerations (Low Priority)

UUID Generation:

  • UUID v4 generation is cryptographically secure but slower than alternatives
  • For high-volume uploads, consider UUID v1 or nanoid for better performance
  • Current implementation is fine for typical use cases

6. Error Handling (Medium Priority)

In sanitize-filename.ts:

  • No error handling for invalid input (null, undefined, empty string)

Recommendation:

export function sanitizeAndUniquifyFilename(originalName: string): string {
    if (!originalName || typeof originalName !== 'string') {
        return uuidv4(); // Return just UUID for invalid input
    }
    // ... rest of implementation
}

7. Code Quality (Low Priority)

Unused imports/dependencies:

  • Several components import unused icons and utilities
  • Consider cleanup to reduce bundle size

🔴 Potential Bugs

1. Runtime Error Risk

The incomplete camelCase → snake_case migration could cause:

  • Property access errors when backend returns snake_case but frontend expects camelCase
  • Undefined property access in chat components

2. Chat Initialization Edge Case

If organizationId becomes available after the initial render but auth is enabled, the chat creation logic might not trigger properly.

📋 Testing Recommendations

Since this touches critical functionality:

  1. Unit Tests Needed:

    • sanitizeAndUniquifyFilename() with various inputs
    • Chat session initialization with different auth states
    • File upload with UUID-renamed files
  2. Integration Tests:

    • End-to-end file upload workflow
    • Chat creation with different authentication scenarios
    • API property access across all components
  3. Manual Testing:

    • Upload files with various extensions
    • Test chat creation with auth enabled/disabled
    • Verify all project/dataset displays show correct timestamps

🎯 Action Items

Must Fix:

  1. Complete the camelCase → snake_case migration across ALL files
  2. Add input validation to sanitizeAndUniquifyFilename()
  3. Clarify authentication logic in chat session initialization

Should Fix:
4. Replace any types with proper interfaces
5. Add file extension validation to sanitization
6. Improve error handling

Could Fix:
7. Clean up unused imports
8. Add comprehensive tests
9. Consider performance optimizations for UUID generation


Overall, this is a solid PR that addresses the core requirements, but the incomplete API migration is a critical issue that must be resolved before merging to prevent runtime errors.

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.

1 participant