Skip to content

✨ feat: Add attachment preview with drag & drop support #2180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 25, 2025

Conversation

kumanoayumi
Copy link
Member

@kumanoayumi kumanoayumi commented Jun 24, 2025

Issue

  • resolve:

Why is this change needed?

This PR implements the complete attachment feature for GitHubSessionFormPresenter:

  • Creates AttachmentPreview component with remove functionality
  • Updates styling to use semantic tokens for design consistency
  • Adds drag & drop support for better UX

What would you like reviewers to focus on?

  • AttachmentPreview component displays images correctly with remove button
  • All styling uses semantic tokens (--spacing-, --border-radius-, --overlay-*, etc.)
  • Drag & drop works with proper visual feedback (border/background changes)
  • Multiple attachments display with correct 8px gap
  • Overall implementation matches Figma design

Testing Verification

https://liam-storybook-git-feat-attachment-preview-liambx.vercel.app/?path=/docs/features-sessions-githubsessionformpresenter--docs

2025-06-24.16.16.42.mov

What was done

🤖 Generated by PR Agent at f1672d9

  • Add AttachmentPreview component with remove functionality
  • Implement drag & drop support for file uploads
  • Update styling to use semantic design tokens
  • Enable multiple attachment display with proper spacing

Detailed Changes

Relevant files
Enhancement
9 files
index.ts
Export AttachmentPreview component                                             
+1/-0     
index.ts
Add X icon export                                                                               
+1/-0     
AttachButton.module.css
Add hidden input styling                                                                 
+4/-0     
AttachmentPreview.module.css
Create attachment preview component styles                             
+52/-0   
GitHubSessionFormPresenter.module.css
Add drag active state and attachments container                   
+14/-1   
AttachButton.tsx
Add file selection and input handling                                       
+56/-23 
AttachmentPreview.tsx
Create attachment preview component with remove button     
+29/-0   
GitHubSessionFormPresenter.tsx
Implement drag & drop and attachment management                   
+62/-2   
SessionFormActions.tsx
Add file selection prop to actions                                             
+3/-0     

Additional Notes


Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    changeset-bot bot commented Jun 24, 2025

    ⚠️ No Changeset found

    Latest commit: df9e294

    Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

    This PR includes no changesets

    When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

    Click here to learn what changesets are, and how to add one.

    Click here if you're a maintainer who wants to add a changeset to this PR

    Copy link

    vercel bot commented Jun 24, 2025

    The latest updates on your projects. Learn more about Vercel for Git ↗︎

    Name Status Preview Comments Updated (UTC)
    liam-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 25, 2025 4:17am
    liam-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 25, 2025 4:17am
    2 Skipped Deployments
    Name Status Preview Comments Updated (UTC)
    liam-docs ⬜️ Ignored (Inspect) Visit Preview Jun 25, 2025 4:17am
    liam-erd-sample ⬜️ Skipped (Inspect) Jun 25, 2025 4:17am

    Copy link

    supabase bot commented Jun 24, 2025

    Updates to Preview Branch (feat/attachment-preview) ↗︎

    Deployments Status Updated
    Database Wed, 25 Jun 2025 04:13:12 UTC
    Services Wed, 25 Jun 2025 04:13:12 UTC
    APIs Wed, 25 Jun 2025 04:13:12 UTC

    Tasks are run on every commit but only new migration files are pushed.
    Close and reopen this PR if you want to apply changes from existing seed or migration files.

    Tasks Status Updated
    Configurations Wed, 25 Jun 2025 04:13:13 UTC
    Migrations Wed, 25 Jun 2025 04:13:13 UTC
    Seeding Wed, 25 Jun 2025 04:13:13 UTC
    Edge Functions Wed, 25 Jun 2025 04:13:13 UTC

    View logs for this Workflow Run ↗︎.
    Learn more about Supabase for Git ↗︎.

    @vercel vercel bot temporarily deployed to Preview – liam-erd-sample June 24, 2025 07:05 Inactive
    @kumanoayumi kumanoayumi changed the title 💄 feat: implement for AttachmentPreview component ✨ feat: Add attachment preview with drag & drop support Jun 24, 2025
    @kumanoayumi kumanoayumi marked this pull request as ready for review June 24, 2025 07:18
    @kumanoayumi kumanoayumi requested a review from a team as a code owner June 24, 2025 07:18
    @kumanoayumi kumanoayumi requested review from hoshinotsuyoshi, FunamaYukina, junkisai, MH4GF and NoritakaIkeda and removed request for a team June 24, 2025 07:18
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Memory Leak

    Object URLs created with URL.createObjectURL() are not properly cleaned up when the component unmounts. This could lead to memory leaks if the component is unmounted while attachments are still present.

      url: URL.createObjectURL(file),
      name: file.name,
    }))
    setAttachments((prev) => [...prev, ...newAttachments])
    
    Event Handling

    The handleClick function calls both the file input click and the original onClick prop, which could lead to unexpected behavior if the original onClick has side effects or prevents default behavior.

    const handleClick = (e: React.MouseEvent<HTMLButtonElement>) => {
      e.preventDefault()
      fileInputRef.current?.click()
      props.onClick?.(e)
    }
    
    Drag State

    The drag state management may not handle edge cases properly, such as when dragging leaves the window or when multiple drag events occur simultaneously, potentially leaving the component in an incorrect drag state.

    const handleDrag = (e: DragEvent) => {
      e.preventDefault()
      e.stopPropagation()
      if (e.type === 'dragenter' || e.type === 'dragover') {
        setDragActive(true)
      } else if (e.type === 'dragleave') {
        setDragActive(false)
      }
    }
    

    Copy link
    Contributor

    qodo-merge-for-open-source bot commented Jun 24, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Fix drag leave flickering

    The drag leave handler can trigger when moving between child elements, causing
    flickering. Check if the related target is still within the container to avoid
    premature deactivation of the drag state.

    frontend/apps/app/features/sessions/components/SessionForm/GitHubSessionFormPresenter/GitHubSessionFormPresenter.tsx [56-64]

     const handleDrag = (e: DragEvent) => {
       e.preventDefault()
       e.stopPropagation()
       if (e.type === 'dragenter' || e.type === 'dragover') {
         setDragActive(true)
       } else if (e.type === 'dragleave') {
    -    setDragActive(false)
    +    const rect = e.currentTarget.getBoundingClientRect()
    +    const x = e.clientX
    +    const y = e.clientY
    +    if (x < rect.left || x > rect.right || y < rect.top || y > rect.bottom) {
    +      setDragActive(false)
    +    }
       }
     }
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a common issue with dragleave events firing when moving over child elements, which can cause UI flickering. The proposed solution of checking the cursor's position against the element's bounding box is a valid and effective way to fix this potential bug and improve the user experience.

    Medium
    Possible issue
    Add array bounds checking

    Add bounds checking to prevent runtime errors when accessing array elements. The
    function should verify that the index is valid before attempting to access or
    remove the attachment.

    frontend/apps/app/features/sessions/components/SessionForm/GitHubSessionFormPresenter/GitHubSessionFormPresenter.tsx [76-83]

     const handleRemoveAttachment = (index: number) => {
       setAttachments((prev) => {
    +    if (index < 0 || index >= prev.length) return prev
         const updated = [...prev]
         URL.revokeObjectURL(updated[index].url)
         updated.splice(index, 1)
         return updated
       })
     }
    • Apply / Chat
    Suggestion importance[1-10]: 4

    __

    Why: The suggestion correctly identifies a potential out-of-bounds access issue in handleRemoveAttachment. While the current implementation makes this unlikely as the index comes from a map function, adding a bounds check is a good defensive programming practice that improves the function's robustness against future changes.

    Low
    • Update

    Copy link
    Member

    @junkisai junkisai left a comment

    Choose a reason for hiding this comment

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

    It seems mostly good!

    kumanoayumi and others added 2 commits June 25, 2025 13:04
    Replace custom CSS class with standard HTML hidden attribute for cleaner code
    
    🤖 Generated with [Claude Code](https://claude.ai/code)
    
    Co-Authored-By: Claude <[email protected]>
    - Use clsx for more readable className composition in GitHubSessionFormPresenter
    - Remove redundant displayName from AttachmentPreview component
    
    🤖 Generated with [Claude Code](https://claude.ai/code)
    
    Co-Authored-By: Claude <[email protected]>
    @kumanoayumi
    Copy link
    Member Author

    @junkisai Thank you for your reviews. I fixed your feedbacks, please review this PR again.

    Copy link
    Member

    @junkisai junkisai left a comment

    Choose a reason for hiding this comment

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

    Thanks for the corrections!

    @junkisai junkisai added this pull request to the merge queue Jun 25, 2025
    Merged via the queue into main with commit a509702 Jun 25, 2025
    29 checks passed
    @junkisai junkisai deleted the feat/attachment-preview branch June 25, 2025 05:45
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants