fix: make notifications error recoverable and harden audio playback - #258
fix: make notifications error recoverable and harden audio playback#258rajivsinclair wants to merge 3 commits into
Conversation
The 'Error loading notifications' card came from the InboxPopover error boundary catching a transient failure of the Liveblocks useInboxNotifications() fetch. The boundary had no recovery path, so a momentary auth/network blip left the inbox stuck on the red error until a full page reload. Add a 'Try again' button that resets the boundary and re-attempts the fetch. The notifications and audio subsystems are independent in code; the audio player only uses AudioContext. Separately harden audio playback so a rejected audio.play() (failed load or blocked playback) no longer leaves the UI showing a 'playing' state or throws an unhandled promise rejection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xzc3UAStnub14PfQGJxhxZ
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 51 minutes and 41 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughTwo audio player components ( ChangesAudio Player Promise-Based State
Inbox Error Boundary Reset
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
✅ Preview Deployment Ready!URL: https://pr-258-verdad-frontend.fly.dev Preview auto-updates on push. Destroyed when PR closes. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the error handling in the inbox popover to use a dedicated error fallback component, and updates the audio player components to handle promise rejections during playback. The review feedback highlights a potential race condition in both audio player components where state is updated asynchronously after playback starts, and suggests setting the playing state synchronously to prevent duplicate playback triggers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| audio.play().then( | ||
| () => setIsPlaying(true), | ||
| () => setIsPlaying(false) | ||
| ) |
There was a problem hiding this comment.
Setting isPlaying to true only after the audio.play() promise resolves introduces a race condition. If a user clicks the play/pause button again while the play promise is still pending, isPlaying is still false, causing another concurrent audio.play() call instead of pausing.
To prevent this, set isPlaying to true synchronously when initiating playback, and revert it to false in the .catch() block if the play request is blocked or fails.
| audio.play().then( | |
| () => setIsPlaying(true), | |
| () => setIsPlaying(false) | |
| ) | |
| setIsPlaying(true) | |
| audio.play().catch(() => { | |
| setIsPlaying(false) | |
| }) |
| void audio.play().catch(() => { | ||
| // play() rejects if the clip fails to load or playback is blocked; | ||
| // the 'play' event never fires, so isPlaying stays false. | ||
| setIsPlaying(false) | ||
| }) |
There was a problem hiding this comment.
Similar to AudioPlayer.tsx, relying entirely on the asynchronous 'play' event to set isPlaying to true introduces a race condition. If the user clicks the play button again before the 'play' event fires, isPlaying is still false, triggering another concurrent audio.play() call.
Setting isPlaying to true synchronously when initiating playback prevents this double-triggering, and the .catch() block will correctly revert it to false if playback fails or is blocked.
setIsPlaying(true)
void audio.play().catch(() => {
setIsPlaying(false)
})
Address review feedback: setting isPlaying only after play() resolves left a window where a rapid second click re-triggered play() instead of pausing. Set isPlaying true synchronously when starting playback and revert it in the catch handler if the clip fails to load or playback is blocked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xzc3UAStnub14PfQGJxhxZ
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a86999e9b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sideOffset={5}> | ||
| <ErrorBoundary | ||
| fallback={<div className='p-3 text-center text-sm text-red-500'>Error loading notifications</div>}> | ||
| <ErrorBoundary FallbackComponent={NotificationsError}> |
There was a problem hiding this comment.
This boundary now exposes a retry button, but the subtree still uses the regular Liveblocks hooks from @liveblocks/react instead of the suspense exports or explicit error/isLoading handling. When useInboxNotifications() is still in an error/loading state, Inbox re-renders with the same unavailable inboxNotifications value and throws again on .length, so resetErrorBoundary immediately returns users to this fallback rather than retrying the notifications fetch. Please switch these notification hooks/ClientSideSuspense to the suspense exports or handle the regular hook state with an actual refetch/reset path.
Useful? React with 👍 / 👎.
The retry button was ineffective: useInboxNotifications from @liveblocks/react is the non-suspense hook, which returns inboxNotifications=undefined in both loading and error states. On error, Inbox threw on .length and resetErrorBoundary just re-rendered into the same cached error, bouncing straight back to the fallback without refetching. Import useInboxNotifications from @liveblocks/react/suspense instead (the LiveblocksProvider is already the suspense provider). Loading now suspends to the ClientSideSuspense 'Loading...' fallback, errors throw to the boundary, and resetting the boundary re-subscribes and refetches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xzc3UAStnub14PfQGJxhxZ
2 participants
Summary
Investigated the reported "Error loading notifications" card that appeared while playing an audio clip.
Root cause / connection: The audio player and the notifications system are not connected in code — the reporter's suspicion that they were unrelated is correct. The audio components (
AudioPlayer,SnippetAudioPlayer) only touch a smallAudioContextand never reference Liveblocks or notifications.The red "Error loading notifications" card is the
InboxPopovererror boundary fallback, triggered when the LiveblocksuseInboxNotifications()Suspense throws — i.e. a transient failure of the notifications fetch (e.g. expired Supabase access token or a network blip in the/api/liveblocks-authcall). The boundary had no recovery path, so a momentary failure left the inbox stuck on the red error until a full page reload. With the inbox popover open while clicking play, the two surfaced at the same time and looked related.Changes
InboxPopover.tsx— Replace the static error fallback with aNotificationsErrorcomponent that includes a "Try again" button.resetErrorBoundaryre-renders the children, re-attempting the Liveblocks notifications fetch, so transient failures recover without a full reload.SnippetAudioPlayer.tsx—audio.play()returns a promise that rejects when a clip fails to load or playback is blocked. Catch the rejection (resettingisPlaying) instead of leaving an unhandled promise rejection.AudioPlayer.tsx— Only flipisPlayingtotrueonceplay()actually resolves; on rejection keep itfalse. Previously the state was toggled unconditionally, so a failed play left the UI showing a "playing" state with nothing playing.Testing
vite buildpasses.npm run lintfails repo-wide on ~1000 pre-existing errors onmain, unrelated to this change.)🤖 Generated with Claude Code
https://claude.ai/code/session_01Xzc3UAStnub14PfQGJxhxZ
Generated by Claude Code
Summary by CodeRabbit