Conversation
|
Feels dangerous enough to me that maybe it should target 6.4 instead? What do you think? |
| } | ||
|
|
||
| // Detector above the canvas so we can detect hover and clicks of playback controls | ||
| .bloom-videoMouseDetector { |
There was a problem hiding this comment.
I don't know what to call this. "videoInteractionDetector"? "videoMouseEventDetector"? If we decide not to preserve hover detection (see below) then we could call it "videoClickDetector"
| &.bloom-videoReplayIcon { | ||
| display: flex; | ||
| .bloom-videoContainer.playing { | ||
| .bloom-videoMouseDetector:hover { |
There was a problem hiding this comment.
Looks like we decided not to do hover behavior in BloomPlayer (e.g. BloomBooks/bloom-player@c31f0fc) but kept it in BloomDesktop. Do we still want this?
| .filter( | ||
| (_, x) => | ||
| !x.classList.contains("bloom-videoControlContainer"), | ||
| !x.classList.contains("bloom-videoControlContainer") && |
There was a problem hiding this comment.
Not sure what's going on here, we have a "Not sure about keeping this" comment above, but now there's no point in keeping a bloom-videoControlContainer without a bloom-videoMouseDetector, it won't work
|
Please retarget to master |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
nabalone
force-pushed
the
BL-15721_put_bubbles_above_videos
branch
from
3ddcf5d to
580c9ac
Compare
|
Ugh devin review is all messed up because I forgot to retarget before having it start. Trying to figure out how to fix it |
I told it to regen. |
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
🐛 2 issues in files not directly in the diff
⚠️ AddInlineScript computes typeAttr but never sets it on the script element (src/BloomExe/Book/HtmlDom.cs:379-385)
The new AddInlineScript method in HtmlDom.cs computes a typeAttr string on line 382 (var typeAttr = module ? "module" : "text/javascript";) but never actually sets it as an attribute on the <script> element. The script element is appended without a type attribute, so the browser will default to text/javascript.
Root Cause and Impact
The caller in Book.cs:579-586 passes module: true to request a module script for Vite dev hot reloading preamble code. Because the type attribute is never set on the element, the browser treats the script as classic text/javascript instead of module. While this specific inline script doesn't use ES module syntax (no import/export), the intent to mark it as a module is lost, and this is clearly unfinished code — the pattern used in the sibling AddScriptFile method (src/BloomExe/Book/HtmlDom.cs:362-374) correctly calls script.SetAttribute("type", typeAttr). The AddInlineScript method should do the same.
🐛 BloomReaderUDPListener.Dispose() dereferences potentially null _clientForBookRequestReceive (src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs:130)
Dispose() at line 130 calls _clientForBookRequestReceive.Dispose() without a null check, but the field is initialized to null at src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs:25 and only assigned inside the async ListenAsync method.
Root Cause and Impact
The constructor (BloomReaderUDPListener()) starts ListenAsync on a background task via Task.Run. If Dispose() is called before ListenAsync has reached line 71 where _clientForBookRequestReceive is assigned — or if ListenAsync returns early because the new UdpClient(...) constructor throws (caught at line 79) — then _clientForBookRequestReceive is still null. Calling .Dispose() on null throws a NullReferenceException, crashing the dispose path.
Additionally, even in the normal flow, ListenAsync wraps the client in a using block (line 85), which will dispose it when the loop exits. Then Dispose() disposes it again — a double-dispose. While UdpClient likely tolerates this, the null case is a real crash risk.
View 9 additional findings in Devin Review.
nabalone
force-pushed
the
BL-15721_put_bubbles_above_videos
branch
from
580c9ac to
97d01c7
Compare
|
I think we should wait until after BL-15935 and BL-15936. If we still think this is PR is even worth doing. |
There was a problem hiding this comment.
Devin Review found 4 new potential issues.
🐛 2 issues in files not directly in the diff
⚠️ AddInlineScript computes typeAttr but never sets it on the script element (src/BloomExe/Book/HtmlDom.cs:379-385)
The new AddInlineScript method in HtmlDom.cs computes a typeAttr string on line 382 (var typeAttr = module ? "module" : "text/javascript";) but never actually sets it as an attribute on the <script> element. The script element is appended without a type attribute, so the browser will default to text/javascript.
Root Cause and Impact
The caller in Book.cs:579-586 passes module: true to request a module script for Vite dev hot reloading preamble code. Because the type attribute is never set on the element, the browser treats the script as classic text/javascript instead of module. While this specific inline script doesn't use ES module syntax (no import/export), the intent to mark it as a module is lost, and this is clearly unfinished code — the pattern used in the sibling AddScriptFile method (src/BloomExe/Book/HtmlDom.cs:362-374) correctly calls script.SetAttribute("type", typeAttr). The AddInlineScript method should do the same.
🐛 BloomReaderUDPListener.Dispose() dereferences potentially null _clientForBookRequestReceive (src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs:130)
Dispose() at line 130 calls _clientForBookRequestReceive.Dispose() without a null check, but the field is initialized to null at src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs:25 and only assigned inside the async ListenAsync method.
Root Cause and Impact
The constructor (BloomReaderUDPListener()) starts ListenAsync on a background task via Task.Run. If Dispose() is called before ListenAsync has reached line 71 where _clientForBookRequestReceive is assigned — or if ListenAsync returns early because the new UdpClient(...) constructor throws (caught at line 79) — then _clientForBookRequestReceive is still null. Calling .Dispose() on null throws a NullReferenceException, crashing the dispose path.
Additionally, even in the normal flow, ListenAsync wraps the client in a using block (line 85), which will dispose it when the loop exits. Then Dispose() disposes it again — a double-dispose. While UdpClient likely tolerates this, the null case is a real crash risk.
View 11 additional findings in Devin Review.
| // can't move them if they aren't draggable, so it | ||
| // makes sense that a click anywhere on the video would play it; there's nothing else useful | ||
| // to do in response. | ||
| if (!video.closest(".drag-activity-play")) { |
There was a problem hiding this comment.
🟡 Potential null dereference on video in handleVideoClick after optional chaining
The handleVideoClick handler obtains video via optional chaining (lines 400-402), meaning video can be undefined. However, line 410 immediately accesses video.closest(...) without a null check, which would throw TypeError: Cannot read properties of undefined (reading 'closest'). Lines 414 and 417 also access video.paused / pass ev to handlers that dereference video without guarding.
Root Cause
Before this PR, video was assigned as ev.currentTarget as HTMLVideoElement (bloomVideo.ts:399 old code), which was always a valid HTMLVideoElement because the click listener was attached directly to the <video> element. After the change, the listener is on the bloom-videoMouseDetector div, and video is obtained through DOM traversal:
const video = (ev.currentTarget as HTMLElement)
?.closest(".bloom-videoContainer")
?.getElementsByTagName("video")[0];The optional chaining (?.) acknowledges that the result can be undefined, but the very next usage at line 410 does not guard against it:
if (!video.closest(".drag-activity-play")) {In normal operation this is unlikely to crash because the mouseDetector is only created when a <video> element exists inside the container. However, if the video element is removed after setup (e.g., by another editing operation), or if the DOM structure is unexpected, this will throw at runtime.
Impact: A crash in the click handler would prevent any further click processing on that video container, silently breaking play/pause behavior.
| if (!video.closest(".drag-activity-play")) { | |
| if (!video || !video.closest(".drag-activity-play")) { | |
Was this helpful? React with 👍 or 👎 to provide feedback.
| // TODO copilot did this and it works but I'm not sure it's the right fix. Let's see if HandlePlayClick gets fixed in BL-15936 | ||
| // For draggable videos in Play Mode, disable pointer events on the mouse detector | ||
| // so clicks can pass through to the drag system which handles playback | ||
| .drag-activity-play [data-draggable-id] .bloom-videoMouseDetector { | ||
| pointer-events: none; | ||
| } |
There was a problem hiding this comment.
🚩 TODO comment left in production code suggesting uncertainty about approach
Lines 1690-1694 contain a TODO comment: // TODO copilot did this and it works but I'm not sure it's the right fix. along with a CSS rule that disables pointer events on the mouse detector for draggable videos in Play Mode. The comment suggests this rule was generated by Copilot and the author isn't confident it's correct. This may need a follow-up investigation per BL-15936 to ensure draggable video playback works properly without this workaround.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
This change is