creates a hasFinished state #9
Conversation
This makes it possible to have a hasFinished useEffect when using it as a Hook
WalkthroughThe changes introduce a new state variable, Changes
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 (2)
src/index.tsx (2)
89-90: LGTM: Event handlers for 'ready' and 'finish' implemented correctly.The changes correctly implement the
hasFinishedstate updates:
- Resetting to
falseon the 'ready' event ensures the correct state when audio is restarted or changed.- Setting to
trueon the 'finish' event captures when audio playback is complete.These implementations align well with the PR objectives.
Consider adding a comment explaining the purpose of the
hasFinishedstate for better code documentation:// Track whether the audio has finished playing const [hasFinished, setHasFinished] = useState<boolean>(false);Also applies to: 93-96
211-211: LGTM: Documentation updated to reflect newhasFinishedstate.The example usage in the documentation has been correctly updated to include
hasFinishedin the destructured return value of theuseWavesurferhook. This change ensures that users of the hook are aware of the new functionality.Consider adding a brief comment in the documentation to explain the purpose of
hasFinished:/** * @public */ export function useWavesurfer({ container, ...options }: Omit<WaveSurferOptions, 'container'> & { container: RefObject<HTMLElement> }): ReturnType< typeof useWavesurferState > & { wavesurfer: ReturnType<typeof useWavesurferInstance> } { // ... existing code ... // Returns an object with wavesurfer instance and state, including: // - isReady: boolean indicating if wavesurfer is ready // - isPlaying: boolean indicating if audio is currently playing // - hasFinished: boolean indicating if audio playback has finished // - currentTime: number representing current playback time return useMemo(() => ({ ...state, wavesurfer }), [state, wavesurfer]) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/index.tsx (4 hunks)
🔇 Additional comments (3)
src/index.tsx (3)
73-73: LGTM: New state variable added as intended.The addition of the
hasFinishedstate variable aligns with the PR objective to capture the 'finish' event. The initial state is correctly set tofalse, and the naming convention is consistent with other state variables.
124-124: LGTM:hasFinishedcorrectly added to return value and dependency array.The changes appropriately integrate the new
hasFinishedstate:
- Adding
hasFinishedto the returned object makes it available for consumers of this hook.- Including
hasFinishedin theuseMemodependency array ensures the memoized value is updated whenhasFinishedchanges.These modifications are consistent with the PR objectives and adhere to React best practices.
Also applies to: 127-127
Line range hint
1-238: Overall assessment: Changes successfully implement thehasFinishedstate.The modifications in this PR effectively introduce the
hasFinishedstate to capture the 'finish' event in the WaveSurfer hook. The implementation is consistent throughout the file, including:
- State initialization
- Event handling for 'ready' and 'finish' events
- Inclusion in the hook's return value
- Update of the
useMemodependency array- Documentation updates
These changes align well with the PR objectives and follow React best practices. The new functionality enhances the usability of the WaveSurfer component by allowing developers to respond to audio playback completion.
A few minor suggestions have been made to improve documentation, but these are not critical for the functionality.
There was a problem hiding this comment.
You can also just subscribe to this event directly in your app:
const { wavesurfer } = useWavesurfer({ ... })
useEffect(() => {
if (!wavesurfer) return
return wavesurfer.on('finish', () => {
// do something
})
}, [wavesurfer])But I'm approving the PR anyway, as it's a useful shortcut.
Thanks!
It doesn't look like it is possible to get the 'finish' event when you are using WaveSurfer as a hook. This PR creates a State for whether the 'finish' event has fired. It is reset when 'ready' event is fired. This makes it possible to have useEffect related to this event.
There maybe a better way of doing this... but this seems to work
Summary by CodeRabbit
hasFinished, to track audio playback completion.