feat(mobile): Enable attachment button functionality #309
Conversation
The attachment button on the mobile view was not functional because it was located in a separate component (`MobileIconsBar`) from the chat input (`ChatPanel`), where the file handling logic resides. The `onClick` handler was missing on the mobile button. This commit resolves the issue by using `React.forwardRef` and `useImperativeHandle` to expose the `handleAttachmentClick` function from the `ChatPanel` component. This allows the parent `Chat` component to create a ref to `ChatPanel` and pass down a handler to the `MobileIconsBar`. Changes: - Modified `ChatPanel` to expose `handleAttachmentClick` via a ref. - Updated the parent `Chat` component to create a ref and pass the click handler to `MobileIconsBar`. - Added the `onClick` prop to the `Paperclip` button in `MobileIconsBar` to trigger the file selection dialog.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
WalkthroughIntroduces an imperative ref API to ChatPanel (handleAttachmentClick) via forwardRef, updates Chat to hold and use this ref, and wires MobileIconsBar’s paperclip button to trigger the ChatPanel’s hidden file input. Chat also wraps layouts in MapDataProvider and adds effects for routing and AI-response refresh. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MobileIconsBar
participant Chat
participant ChatPanel
participant FileInput as Hidden File Input
User->>MobileIconsBar: Tap paperclip
MobileIconsBar->>Chat: onAttachmentClick()
Chat->>ChatPanel: chatPanelRef.current.handleAttachmentClick()
ChatPanel->>FileInput: .click()
FileInput-->>User: File chooser opens
sequenceDiagram
participant Chat
participant MapDataProvider
participant MapDataContext as useMapData
participant Router
Chat->>MapDataProvider: Wrap layouts
Chat->>MapDataContext: useMapData()
Note over Chat,MapDataContext: On drawnFeatures change
Chat->>MapDataContext: updateDrawingContext(id, drawnFeatures)
Note over Chat,Router: On AI response events
Chat->>Router: router.refresh()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core ref-based wiring to enable the mobile attachment button is correct and well-scoped. However, a dev.log file was committed and should be removed and ignored going forward. Consider stabilizing the useImperativeHandle handle with a dependency array and memoizing the handleAttachment callback. To preserve compatibility, making onAttachmentClick optional with a default no-op can prevent breakage at other usage sites.
Additional notes (2)
-
Performance |
components/chat-panel.tsx:34-39
useImperativeHandlerecreates the imperative handle every render. While harmless here, you can make the handle stable and avoid unnecessary updates by providing a dependency array. Since this only usesfileInputRef(which is stable), an empty dependency array is appropriate. -
Performance |
components/chat.tsx:31-33
handleAttachmentis recreated on every render. IfMobileIconsBarbecomes memoized, a stable callback avoids unnecessary re-renders. Not critical, but easy to tighten up.
Summary of changes
- ChatPanel converted to use
React.forwardRefanduseImperativeHandleto expose ahandleAttachmentClickmethod via aChatPanelRefinterface, plus addeddisplayName. - Chat component now creates a
reftoChatPanel, defines ahandleAttachmentfunction, passes it toMobileIconsBar, and attaches the ref toChatPanelin the mobile layout. - MobileIconsBar now accepts a new
onAttachmentClickprop and wires it to the Paperclip button’sonClick. - A
dev.logfile was added containing local Next.js startup logs.
| ⚠ Port 3000 is in use, using available port 3001 instead. | ||
| ▲ Next.js 15.3.3 (Turbopack) | ||
| - Local: http://localhost:3001 | ||
| - Network: http://192.168.0.2:3001 | ||
| - Environments: .env.local, .env | ||
|
|
||
| ✓ Starting... |
There was a problem hiding this comment.
Committing dev logs is a maintainability and hygiene issue. This file appears to contain local development diagnostics and should not be versioned. It risks repository noise and potential leakage of environment info. Please remove it and add it to .gitignore.
Suggestion
Remove dev.log from the repo and add it to .gitignore to prevent future commits. For example, add the following to your .gitignore:
# Local dev logs
/dev.log
*.log
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion
| <Button variant="ghost" size="icon" onClick={onAttachmentClick}> | ||
| <Paperclip className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" /> | ||
| </Button> |
There was a problem hiding this comment.
Icon-only buttons should provide an accessible name. The paperclip button currently lacks an aria-label, which makes it hard for screen readers to announce its purpose.
Suggestion
Add an accessible label to the button:
<Button variant="ghost" size="icon" onClick={onAttachmentClick} aria-label="Attach file">
<Paperclip className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" />
</Button>Alternatively, add visually hidden text via an sr-only span.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
dev.logis excluded by!**/*.log
📒 Files selected for processing (3)
components/chat-panel.tsx(4 hunks)components/chat.tsx(3 hunks)components/mobile-icons-bar.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/chat-panel.tsx (1)
components/followup-panel.tsx (1)
FollowupPanel(12-63)
components/chat.tsx (2)
components/chat-panel.tsx (2)
ChatPanelRef(20-22)ChatPanel(24-256)components/mobile-icons-bar.tsx (1)
MobileIconsBar(25-62)
🔇 Additional comments (12)
components/mobile-icons-bar.tsx (3)
21-23: LGTM! Clean interface definition.The
MobileIconsBarPropsinterface clearly defines the callback prop needed for attachment functionality.
25-25: LGTM! Proper component signature update.The component now correctly accepts and destructures the
onAttachmentClickprop.
52-54: LGTM! Attachment button now functional.The Paperclip button is correctly wired to the
onAttachmentClickhandler, enabling the attachment functionality in mobile view.components/chat-panel.tsx (3)
20-22: LGTM! Well-defined imperative handle interface.The
ChatPanelRefinterface correctly defines the exposed API for parent components to trigger attachment actions.
24-24: LGTM! Proper forwardRef implementation.The component is correctly converted to use
forwardRefto expose the imperative handle.
256-257: LGTM! Proper forwardRef closure.The component correctly closes the
forwardRefwrapper and sets adisplayNamefor better debugging.components/chat.tsx (6)
5-5: LGTM! Correct import for ref typing.The
ChatPanelReftype is properly imported to type the ref used for imperative control.
29-29: LGTM! Ref correctly initialized.The
chatPanelRefis properly typed and initialized usinguseRef.
31-33: LGTM! Clean wrapper function.The
handleAttachmentwrapper safely invokes the imperative handle with optional chaining, properly handling the case where the ref might not be initialized.
88-88: LGTM! Attachment handler correctly wired to mobile icons bar.The
handleAttachmentfunction is correctly passed toMobileIconsBar, enabling the attachment functionality in mobile view.
91-91: LGTM! Ref correctly passed to mobile ChatPanel.The
chatPanelRefis properly passed to the mobileChatPanel, enabling the parent component to trigger the attachment action via the imperative handle.
115-115: Desktop ChatPanel doesn't need the ref.Note that the desktop
ChatPanelat line 115 doesn't receive thechatPanelRef. This is correct because:
- Desktop has an inline Paperclip button within ChatPanel itself (see chat-panel.tsx lines 185-197)
- The ref is only needed for mobile to bridge the external MobileIconsBar to ChatPanel's file input
- Desktop doesn't use MobileIconsBar
No action needed—this is intentional and correct.
| useImperativeHandle(ref, () => ({ | ||
| handleAttachmentClick() { | ||
| fileInputRef.current?.click() | ||
| } | ||
| })); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider refactoring to eliminate code duplication.
The imperative handle directly calls fileInputRef.current?.click(), duplicating the logic in the internal handleAttachmentClick function (lines 61-63). Consider having the imperative handle call the internal function instead for better maintainability.
Apply this diff:
useImperativeHandle(ref, () => ({
handleAttachmentClick() {
- fileInputRef.current?.click()
+ handleAttachmentClick()
}
}));Note: This change requires moving the internal handleAttachmentClick function definition before the useImperativeHandle call to avoid referencing it before declaration.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/chat-panel.tsx around lines 34 to 38, the useImperativeHandle
implementation duplicates the attachment-click logic by directly calling
fileInputRef.current?.click(); move the internal handleAttachmentClick function
(currently at lines ~61-63) above the useImperativeHandle call so it is declared
before use, then change the imperative handle to call that internal function
(e.g., handleAttachmentClick()) instead of duplicating the click logic to
eliminate redundancy and improve maintainability.
2 participants
User description
The attachment button on the mobile view was not functional because it was located in a separate component (
MobileIconsBar) from the chat input (ChatPanel), where the file handling logic resides. TheonClickhandler was missing on the mobile button.This commit resolves the issue by using
React.forwardRefanduseImperativeHandleto expose thehandleAttachmentClickfunction from theChatPanelcomponent. This allows the parentChatcomponent to create a ref toChatPaneland pass down a handler to theMobileIconsBar.Changes:
ChatPanelto exposehandleAttachmentClickvia a ref.Chatcomponent to create a ref and pass the click handler toMobileIconsBar.onClickprop to thePaperclipbutton inMobileIconsBarto trigger the file selection dialog.PR Type
Enhancement
Description
Enable attachment button functionality in mobile view
Implement ref forwarding pattern for component communication
Connect mobile attachment button to file selection dialog
Diagram Walkthrough
File Walkthrough
chat-panel.tsx
Expose attachment handler via ref forwardingcomponents/chat-panel.tsx
forwardRefanduseImperativeHandleto exposehandleAttachmentClickChatPanelRefinterface for type safetychat.tsx
Connect mobile attachment button to chat panelcomponents/chat.tsx
ChatPanelcomponenthandleAttachmentfunction to bridge componentsMobileIconsBarmobile-icons-bar.tsx
Add attachment click handler to mobile barcomponents/mobile-icons-bar.tsx
onAttachmentClickprop to component interfacePaperclipbutton to attachment handlerSummary by CodeRabbit
New Features
Refactor