feat: add audio playback support for WAV and MP3 attachments #5709
Conversation
WalkthroughThe changes introduce an audio playback feature to the Changes
Possibly related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/core/components/issues/attachment/attachment-list-item.tsx (1)
Line range hint
1-105: Consider broader component improvementsWhile the audio player addition is a good feature, there are a few other areas where we could improve the overall component:
Memoization: Consider wrapping the component with
React.memo()to prevent unnecessary re-renders, especially if this component is used in a list of many attachments.Separation of Concerns: The attachment deletion functionality could potentially be moved to a separate component. This would make the
IssueAttachmentsListItemcomponent more focused and easier to maintain.TypeScript Improvements: It appears that the
attachmentobject is using theanytype. Consider defining a specific interface for the attachment object to improve type safety and code readability.Here's a sketch of how these improvements might look:
import React from 'react'; interface Attachment { id: string; asset: string; attributes: { name: string; size: number; }; updated_by: string; updated_at: string; // Add other necessary properties } interface IssueAttachmentsListItemProps { attachmentId: string; disabled?: boolean; } const IssueAttachmentsListItem: React.FC<IssueAttachmentsListItemProps> = observer((props) => { // ... existing component logic ... const attachment: Attachment | undefined = attachmentId ? getAttachmentById(attachmentId) : undefined; // ... rest of the component ... }); export default React.memo(IssueAttachmentsListItem);This refactoring:
- Adds a specific
Attachmentinterface.- Uses the
React.FCtype for better prop typing.- Wraps the component with
React.memo()for potential performance improvements.Consider implementing these changes to improve the overall quality and maintainability of the component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- web/core/components/issues/attachment/attachment-list-item.tsx (1 hunks)
🔇 Additional comments (1)
web/core/components/issues/attachment/attachment-list-item.tsx (1)
62-70: 🛠️ Refactor suggestionImprove audio player implementation and error handling
The addition of an audio player for WAV and MP3 files is a good feature. However, there are several areas where we can improve the implementation:
- The file extension check is case-sensitive. Consider using a case-insensitive comparison to catch uppercase extensions as well.
- The audio element's styling is done inline. It's generally better to use CSS classes for styling to improve maintainability.
- The audio type is hardcoded to "audio/wav" even for MP3 files. This should be dynamic based on the file type.
- There's no error handling if the audio file fails to load.
- Consider adding accessibility attributes to the audio player.
Here's a suggested refactoring that addresses these issues:
- {(getFileExtension(attachment.asset) === "wav" || getFileExtension(attachment.asset) == "mp3") && ( + {/^(wav|mp3)$/i.test(getFileExtension(attachment.asset)) && ( <> - <audio controls style={{height: "20px"}}> - <source src={attachment.asset} type="audio/wav"/> + <audio + controls + className="h-5" + onError={(e) => console.error("Error loading audio:", e)} + aria-label={`Audio player for ${attachment.attributes.name}`} + > + <source + src={attachment.asset} + type={`audio/${getFileExtension(attachment.asset).toLowerCase()}`} + /> Your browser does not support the audio element. </audio> </> ) }This refactoring:
- Uses a case-insensitive regex for file extension checking.
- Replaces inline styling with a className (you'll need to define this class in your CSS).
- Dynamically sets the audio type based on the file extension.
- Adds basic error handling with a console.error (you might want to handle this more gracefully in a production environment).
- Adds an aria-label for improved accessibility.
To ensure we're not missing any other audio file types that might be supported, let's check the codebase for other references to audio files:
This will help us identify if we need to support additional audio formats in the future.
|
Bump? Any feedback on this PR? |
|
Hello @jamesrusso, We've reviewed your changes across different browsers and identified some necessary adjustments for Safari. Our team will handle these modifications and conduct final validations. Once everything is confirmed to work smoothly on all browsers, including Safari, we'll merge your pull request. Thank you for your contribution. |
|
Hey team. Is this feature still progressing? |
This PR adds audio control support. I've leaving this as a draft because I'll take any feedback especially around styling or any other feedback others might have.
Our use case is to listen to voicemail messages from the attachments. This should only impact audio files (wav and mp3).
Summary by CodeRabbit