fix(watch): hide buffering overlay while offline#1410
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis change threads an "offline" Signal from the MoqWatch component into MultiBackend and into Video.Renderer. Renderer now skips normal frame drawing and fills the canvas black when offline. The UI adds an OfflineIndicator component, integrates it into the video container, and updates BufferingIndicator to hide while offline. CSS was added/updated for the offline badge and to center the buffering overlay. 🚥 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)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
js/watch/src/ui/styles/buffering-indicator.css (1)
3-5: ⚡ Quick winRemove redundant
flex--centerclass from component.The CSS now explicitly handles flexbox centering for
.watch-ui__buffering, but the component still applies theflex--centerutility class (line 10 of BufferingIndicator.tsx). This creates redundancy where two sources control the same layout behavior.Since you've added explicit centering to the component's CSS, remove the
flex--centerclass from the component:♻️ Suggested cleanup
In
js/watch/src/ui/components/BufferingIndicator.tsx:- <div class="watch-ui__buffering flex--center"> + <div class="watch-ui__buffering">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/watch/src/ui/styles/buffering-indicator.css` around lines 3 - 5, The component is applying the redundant utility class "flex--center" while the CSS selector ".watch-ui__buffering" already provides flex centering; open the BufferingIndicator component (BufferingIndicator.tsx) and remove the "flex--center" class from the element that renders the buffering UI so only the ".watch-ui__buffering" styling controls layout, then run a quick UI check to ensure centering remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@js/watch/src/ui/styles/buffering-indicator.css`:
- Around line 3-5: The component is applying the redundant utility class
"flex--center" while the CSS selector ".watch-ui__buffering" already provides
flex centering; open the BufferingIndicator component (BufferingIndicator.tsx)
and remove the "flex--center" class from the element that renders the buffering
UI so only the ".watch-ui__buffering" styling controls layout, then run a quick
UI check to ensure centering remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae3da5d3-b927-49da-bab5-8258ca9cf3c4
📒 Files selected for processing (2)
js/watch/src/ui/components/BufferingIndicator.tsxjs/watch/src/ui/styles/buffering-indicator.css
|
Hmm I definitely think we hide the buffering icon, but we should also stop rendering the most recent frame right? A black screen is more obvious than a frozen BBB image. |
When the watch broadcast goes offline, stop rendering the most recent frame and display a black screen instead. This provides a clearer visual indicator that the connection is lost, rather than showing a frozen frame. Changes: - Add offline signal to Renderer to clear canvas when offline - Pass offline status from MultiBackend to Renderer - Track offline status in MoqWatch element based on broadcast status
Display a small black badge with white OFFLINE text in the bottom right corner when the broadcast goes offline. This provides a clear and prominent visual indicator of the offline state. - New OfflineIndicator component that shows when watchStatus is offline - Styling with responsive sizing for different screen sizes - Positioned as a floating badge in the bottom right corner
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/watch/src/ui/styles/offline-indicator.css`:
- Around line 42-43: The .watch-ui__offline-text rule uses font-size: 0.65rem
which is likely too small on mobile; update the CSS for the
.watch-ui__offline-text selector to increase the minimum font-size to 0.75rem
(or add a mobile media query that sets .watch-ui__offline-text to 0.75rem for
small viewports) so the offline status text is more readable on mobile devices.
- Around line 1-13: The offline indicator's stacking context is too low; update
the .watch-ui__offline-indicator rule to increase its z-index from 1 to at least
4 so it sits above buffer control elements (which use z-index 2 and 3). Locate
the .watch-ui__offline-indicator CSS selector and change the z-index value to 4
(or higher if needed) ensuring the alert remains visible above other UI
controls.
In `@js/watch/src/video/renderer.ts`:
- Around line 117-124: When detecting offline in the renderer (the block where
effect.get(this.offline) is true and you draw the black canvas), also clear the
cached frame state so stale values don't prevent decoding after reconnect: reset
this.frame and this.timestamp (the fields used by `#runEnabled`()) before
returning from that offline branch so reconnecting while paused won't keep
decoding disabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07f4e56c-0eff-4355-9825-8dddd12ed1ce
📒 Files selected for processing (7)
js/watch/src/backend.tsjs/watch/src/element.tsjs/watch/src/ui/components/OfflineIndicator.tsxjs/watch/src/ui/element.tsxjs/watch/src/ui/styles/index.cssjs/watch/src/ui/styles/offline-indicator.cssjs/watch/src/video/renderer.ts
✅ Files skipped from review due to trivial changes (3)
- js/watch/src/ui/components/OfflineIndicator.tsx
- js/watch/src/ui/styles/index.css
- js/watch/src/ui/element.tsx
| .watch-ui__offline-indicator { | ||
| position: absolute; | ||
| bottom: 1rem; | ||
| right: 1rem; | ||
| padding: 0.5rem 1rem; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| background-color: #000000; | ||
| z-index: 1; | ||
| pointer-events: auto; | ||
| border-radius: 0.25rem; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check z-index values in watch UI stylesheets to ensure offline indicator is visible above other elements.
# Search for z-index declarations in watch UI styles
rg -n 'z-index:' --glob 'js/watch/src/ui/styles/*.css' -C 2Repository: moq-dev/moq
Length of output: 2072
Increase z-index of the offline indicator above buffer control elements.
The z-index: 1 is too low. Buffer control elements have z-index: 2 and z-index: 3, which means they would appear above the offline indicator. Since this indicator communicates critical status information (broadcast offline), it should be visible above all UI controls. Consider setting z-index to at least 4 to ensure it's not obscured.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/watch/src/ui/styles/offline-indicator.css` around lines 1 - 13, The
offline indicator's stacking context is too low; update the
.watch-ui__offline-indicator rule to increase its z-index from 1 to at least 4
so it sits above buffer control elements (which use z-index 2 and 3). Locate the
.watch-ui__offline-indicator CSS selector and change the z-index value to 4 (or
higher if needed) ensuring the alert remains visible above other UI controls.
| .watch-ui__offline-text { | ||
| font-size: 0.65rem; |
There was a problem hiding this comment.
Consider increasing minimum font size for better mobile readability.
The font-size: 0.65rem (approximately 10.4px) may be too small for comfortable reading on mobile devices, especially for important status information like "offline." Consider using a minimum of 0.75rem to improve readability.
📱 Proposed fix for better mobile readability
.watch-ui__offline-text {
- font-size: 0.65rem;
+ font-size: 0.75rem;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .watch-ui__offline-text { | |
| font-size: 0.65rem; | |
| .watch-ui__offline-text { | |
| font-size: 0.75rem; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/watch/src/ui/styles/offline-indicator.css` around lines 42 - 43, The
.watch-ui__offline-text rule uses font-size: 0.65rem which is likely too small
on mobile; update the CSS for the .watch-ui__offline-text selector to increase
the minimum font-size to 0.75rem (or add a mobile media query that sets
.watch-ui__offline-text to 0.75rem for small viewports) so the offline status
text is more readable on mobile devices.
| const offline = effect.get(this.offline); | ||
|
|
||
| // Clear canvas when offline. | ||
| if (offline) { | ||
| ctx.fillStyle = "#000"; | ||
| ctx.fillRect(0, 0, ctx.canvas.width, ctx.canvas.height); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Clear cached frame state when entering offline mode.
Line 120 returns after painting black but leaves this.frame and this.timestamp intact. Because #runEnabled() checks this.frame when paused (Line 108), reconnecting while paused can keep decoding disabled and preserve stale pre-offline state.
Proposed fix
// Clear canvas when offline.
if (offline) {
+ this.frame.update((old) => {
+ old?.close();
+ return undefined;
+ });
+ this.timestamp.set(undefined);
ctx.fillStyle = "`#000`";
ctx.fillRect(0, 0, ctx.canvas.width, ctx.canvas.height);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const offline = effect.get(this.offline); | |
| // Clear canvas when offline. | |
| if (offline) { | |
| ctx.fillStyle = "#000"; | |
| ctx.fillRect(0, 0, ctx.canvas.width, ctx.canvas.height); | |
| return; | |
| } | |
| const offline = effect.get(this.offline); | |
| // Clear canvas when offline. | |
| if (offline) { | |
| this.frame.update((old) => { | |
| old?.close(); | |
| return undefined; | |
| }); | |
| this.timestamp.set(undefined); | |
| ctx.fillStyle = "`#000`"; | |
| ctx.fillRect(0, 0, ctx.canvas.width, ctx.canvas.height); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/watch/src/video/renderer.ts` around lines 117 - 124, When detecting
offline in the renderer (the block where effect.get(this.offline) is true and
you draw the black canvas), also clear the cached frame state so stale values
don't prevent decoding after reconnect: reset this.frame and this.timestamp (the
fields used by `#runEnabled`()) before returning from that offline branch so
reconnecting while paused won't keep decoding disabled.
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
I think you're very close.
Can you look into Decoder and figure out what it does when a broadcast goes offline? Maybe it's just not unsetting the current frame. I think that's a better fix than a signal that toggles a black screen.
| paused: Signal<boolean>; | ||
|
|
||
| // Whether the watch is offline. | ||
| offline: Signal<boolean>; |
There was a problem hiding this comment.
Maybe flip it and use enabled instead? To match other components.
There was a problem hiding this comment.
Actually, maybe the decoder should unset the frame (on offline) instead of telling the renderer to draw black? IDK.
2 participants
Summary
Fix the watch UI buffering overlay so it does not cover the player while the broadcast is offline.
Changes
watchStatus()isofflineWhy
Issue #737 reports that the buffering UI covers the element and controls even when nothing is loading because the broadcast is offline. In that state, showing a spinner is misleading and blocks the offline state from being communicated clearly.
Testing
bun run checkinjs/watch, but local workspace dependencies were not fully installed, sotscwas unavailable in this environmentCloses #737