Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR refactors the drag-state tracking mechanism in the UiService. The implementation replaces a screen-position heuristic approach with a 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/admin/app/services/ui.js (1)
203-209: ⚡ Quick winCall
resetDragState()at the start of cleanup to clear drag state.
cleanupBodyDragHandlers()only detaches listeners. If cleanup runs while a drag is active, thedata-user-is-draggingflag stays set on the body with no remaining handler able to clear it, leaving dropzone styling stuck. Explicitly callingresetDragState()ensures cleanup is idempotent regardless of drag state.Proposed fix
`@action` cleanupBodyDragHandlers() { + this.resetDragState(); document.body.removeEventListener('dragenter', this.bodyDragEnterHandler, {capture: true}); document.body.removeEventListener('dragleave', this.bodyDragLeaveHandler, {capture: true}); document.body.removeEventListener('dragend', this.resetDragState, {capture: true}); document.body.removeEventListener('drop', this.resetDragState, {capture: true}); window.removeEventListener('blur', this.windowBlurHandler); document.removeEventListener('visibilitychange', this.visibilityChangeHandler); }🤖 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 `@ghost/admin/app/services/ui.js` around lines 203 - 209, Call resetDragState() at the start of cleanupBodyDragHandlers so any active drag state (e.g. the data-user-is-dragging flag on document.body) is cleared before listeners are removed; update cleanupBodyDragHandlers to invoke this.resetDragState() as the first statement, then proceed to remove bodyDragEnterHandler, bodyDragLeaveHandler, resetDragState (listener), drop listener, windowBlurHandler, and visibilityChangeHandler to make cleanup idempotent and avoid stuck dropzone styling.
🤖 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 `@ghost/admin/app/services/ui.js`:
- Around line 203-209: Call resetDragState() at the start of
cleanupBodyDragHandlers so any active drag state (e.g. the data-user-is-dragging
flag on document.body) is cleared before listeners are removed; update
cleanupBodyDragHandlers to invoke this.resetDragState() as the first statement,
then proceed to remove bodyDragEnterHandler, bodyDragLeaveHandler,
resetDragState (listener), drop listener, windowBlurHandler, and
visibilityChangeHandler to make cleanup idempotent and avoid stuck dropzone
styling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 326fcd4e-1758-4ad4-88d5-a1c367629a6e
📒 Files selected for processing (1)
ghost/admin/app/services/ui.js
ref https://linear.app/ghost/issue/ONC-1746/ - in Safari, the "Add feature image" button on posts could stop responding to clicks and stop showing the hand cursor on hover, with the only fix being to refresh the page - the editor uses an invisible 175px dropzone overlay above the button that becomes click-receptive only while a drag is in progress, toggled by `body[data-user-is-dragging]` - the attribute was set on `dragenter` and cleared on `dragend`/`drop`/document-leave `dragleave`. Safari does not reliably fire `dragend` when a drag is released outside the window, leaving the attribute set indefinitely so the overlay permanently swallowed clicks on the button beneath - replaced the boolean + unreliable `screenX/Y === 0` "left the document" heuristic with a depth counter so nested `dragenter`/`dragleave` events stay balanced - added `window.blur` and `document.visibilitychange` listeners as safety nets that hard-reset state - these recover the next time the user switches apps or tabs, even when Safari has dropped the terminal drag event - verified in Safari: dragging a file from Finder into the editor and releasing outside the window reproduces the stuck state without the fix and recovers on focus change with the fix; manual and programmatic stuck states both clear via blur/visibility; normal drag/drop and nested drag flows are unaffected
ref https://linear.app/ghost/issue/ONC-1746/
body[data-user-is-dragging]dragenterand cleared ondragend/drop/document-leavedragleave. Safari is known to skipdragendwhen the drag source is removed from the DOM mid-drag (e.g. autosave re-render) or when the drag is released outside the window, leaving the attribute set indefinitely so the overlay permanently swallowed clicks on the button beneathscreenX/Y === 0"left the document" heuristic with a depth counter so nesteddragenter/dragleaveevents stay balancedwindow.bluranddocument.visibilitychangelisteners as safety nets that hard-reset state - these recover the next time the user switches apps or tabs, even when Safari has dropped the terminal drag event