Dropping files crashes when the editor is torn down or the selection can't accept them by jorgemanrubia · Pull Request #1107 · basecamp/lexxy

jorgemanrubia · 2026-06-10T14:56:26Z

Dropping files onto Basecamp pages crashes when the drop lands on an editor that has been torn down, or while the current selection can't accept the insertion. Fixes BC3-JS-N7D5 — 583 events / 533 users in 3 days, all funneling through Basecamp's document-level drop zone into Contents.uploadFiles. Bug card: 9982374185.

The Sentry issue groups four variants of the same drop path:

Cannot read properties of null (reading 'supportsAttachments') — Contents.uploadFiles called on a disposed Contents: dispose() nulls editorElement, but the editor element still exposes the disposed instance via element.contents, so a drop landing right after teardown (Turbo morph/navigation) crashes. Now handled deliberately as a no-op.
Cannot read properties of undefined (reading 'getNode') — ShadowRootNodeInserter.handles read selection?.anchor.getNode(); node selections (e.g. an attachment selected when the drop happens) have no anchor. Fixed the optional chaining so NodeSelectionNodeInserter gets to handle them, as intended.
Cannot read properties of null (reading 'splice') — GalleryUploader.#findOrCreateGallery assumed $findOrCreateGalleryForImage always returns a gallery, but it returns null when the resolved node can't join one. It now falls back to creating a fresh gallery at the cursor.
Cannot read properties of null (reading 'selectEnd') — CodeNodeInserter.insertNodes assumed a node at the caret after inserting; guarded.

Tests

Three of the four regressions are covered by Playwright tests in test/browser/tests/attachments/upload_failure_guards.test.js, each driving a real editor in a real browser:

Disposed Contents — remove the editor element (real teardown), then call Contents#uploadFiles on the still-exposed instance, the way document-level drop zones do.
Node selection — click a file attachment so it's node-selected, then paste another file; it now lands after the attachment instead of crashing.
Empty code block — place the caret in an empty code block and upload a file through Contents#uploadFiles; previously the null caret crashed CodeNodeInserter (covers Copilot's request for a regression test on this path).

The gallery-splice case stays a unit test in test/javascript/unit/editor/upload_files.test.js: the state it exercises — a selection whose first node is a previewable image while its anchor resolves to a node that can't join a gallery — comes from stale selection restoration during drag-and-drop races and can't be constructed organically; Lexical normalizes the anchor of every selection you can produce in a browser onto the attachment itself (verified by probing backwards, forward-from-root, and select-all selections). Per Copilot's note, its stub now resolves the node at call time inside the calling update instead of reusing a node captured from an earlier read.

Red/green verified by reverting the three src/ files to main: all 3 browser tests fail (the node-selection one with no insertion, the other two with the exact production TypeErrors) and the unit test fails with Cannot read properties of null (reading 'splice'); with the fix restored, 3/3 browser tests pass (chromium and firefox locally) and 1/1 unit test passes.

Also fixed the does not supports attachments console-warning grammar flagged by Copilot.

Copilot

Pull request overview

This PR hardens the editor’s file-drop/upload path against teardown and selection edge-cases that were causing production TypeErrors, primarily by adding defensive guards and correcting inserter/gallery fallback behavior.

Changes:

Make Contents#uploadFiles a no-op when called on a disposed Contents (late drop after teardown).
Ensure gallery uploads fall back to creating a new gallery when $findOrCreateGalleryForImage returns null.
Prevent crashes in node insertion by handling node selections correctly and guarding selectEnd() when the caret resolves to null.
Add unit regression tests for several of the reported crash variants.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
test/javascript/unit/editor/upload_files.test.js	Adds unit tests covering disposed-contents uploads, gallery fallback behavior, and node-selection insertion.
src/editor/contents/uploader.js	Adds a null-safe gallery fallback when the selected/resolved node can’t join a gallery.
src/editor/contents/node_inserter.js	Fixes selection handling for shadow roots and guards code insertion selection updates.
src/editor/contents.js	Adds a guard to prevent `uploadFiles` from running after `Contents` disposal.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jorgemanrubia · 2026-06-10T19:08:29Z

    }

-    caret.getNodeAtCaret().selectEnd()
+    caret.getNodeAtCaret()?.selectEnd()


🤖 Added — test/browser/tests/attachments/upload_failure_guards.test.js now covers this path: with the caret in an empty code block, Contents#uploadFiles (the same entry point document-level drop zones use) used to crash with Cannot read properties of null (reading 'selectEnd'). Verified red against main's source and green with the fix.

I suspect the bug might be on L39.

if (!node.isAttached()) continue

If there's no caret.getNodeAtCaret() then no nodes were inserted into the CodeNode: I'd suspect the filter is removing all the eligible nodes.

jorgemanrubia · 2026-06-10T19:08:32Z

+    let paragraph
+    editorElement.editor.read(() => { paragraph = $getRoot().getFirstChild() })
+
+    // Selection reports a previewable image but resolves to a node that can't join a gallery
+    Object.defineProperty(editorElement.selection, "isOnPreviewableImage", { get: () => true })
+    editorElement.selection.selectedNodeWithOffset = () => ({ node: paragraph, offset: 0 })


🤖 Done — the stub now resolves the node at call time inside the calling update (() => ({ node: $getRoot().getFirstChild(), offset: 0 })) instead of reusing a node captured from an earlier read. This case stays a unit test: the state it exercises (selection whose first node is a previewable image while the anchor resolves to a node that can't join a gallery) comes from stale selection restoration during drag-and-drop races and can't be constructed organically in a browser — Lexical normalizes the anchor of any drivable selection onto the attachment itself. The other regression cases moved to browser tests in test/browser/tests/attachments/upload_failure_guards.test.js.

Dropping files could throw in three ways, all surfaced by Sentry BC3-JS-N7D5 from Basecamp's document-level drop zone: - Contents.uploadFiles ran against a disposed Contents (editorElement nulled after editor teardown), throwing on supportsAttachments. - ShadowRootNodeInserter.handles assumed the selection had an anchor, throwing for node selections (e.g. an attachment selected on drop). - GalleryUploader assumed $findOrCreateGalleryForImage always returned a gallery, throwing on splice when the resolved node couldn't join one. It now falls back to creating a fresh gallery, and CodeNodeInserter tolerates a missing node at the caret after insert.

samuelpecher

I wonder if the core bug here is upstream in the dropzone implementation rather than here: insertingFiles in a torn-down editor seems like implementation error.

The CodeNodeInserter fix might be in the wrong place

samuelpecher · 2026-06-11T13:27:18Z

+    // $findOrCreateGalleryForImage returns null when the resolved node can't join a gallery
+    if (!this.#gallery) {


This feels like masking an error within #selectionIsAfterGalleryEdge which calls ImageGalleryNode.canCollapseWith(selection.nodeBeforeCursor). The logic would dictate that the selection.nodeBeforeCursor should be a gallery-collapsible node

samuelpecher · 2026-06-11T13:30:49Z

    }

-    caret.getNodeAtCaret().selectEnd()
+    caret.getNodeAtCaret()?.selectEnd()


I suspect the bug might be on L39.

if (!node.isAttached()) continue

If there's no caret.getNodeAtCaret() then no nodes were inserted into the CodeNode: I'd suspect the filter is removing all the eligible nodes.

samuelpecher · 2026-06-11T13:32:15Z

  }

  uploadFiles(files, { selectLast } = {}) {
+    if (!this.editorElement) return // Disposed after editor teardown; a late drop can still land here


Is this a potentially swallowing an implementation error where we shouldn't be pasting files into a torn-down editor from Dropzone?

Copilot AI review requested due to automatic review settings June 10, 2026 14:56

Copilot started reviewing on behalf of jorgemanrubia June 10, 2026 14:56 View session

jorgemanrubia marked this pull request as draft June 10, 2026 14:59

Copilot AI reviewed Jun 10, 2026

View reviewed changes

This was referenced Jun 10, 2026

Pasting into a blank list item inside a blockquote crashes with Lexical error #66 #1108

Open

Toggling a code block crashes when the selection spans nested structures #1110

Open

jorgemanrubia force-pushed the sentry-drop-zone-null-editor branch from a3707ef to 0ef6ddc Compare June 10, 2026 19:08

jorgemanrubia marked this pull request as ready for review June 11, 2026 05:20

jorgemanrubia requested a review from samuelpecher June 11, 2026 05:53

samuelpecher reviewed Jun 11, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dropping files crashes when the editor is torn down or the selection can't accept them#1107

Dropping files crashes when the editor is torn down or the selection can't accept them#1107
jorgemanrubia wants to merge 1 commit into
mainfrom
sentry-drop-zone-null-editor

jorgemanrubia commented Jun 10, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

jorgemanrubia Jun 10, 2026

Uh oh!

samuelpecher Jun 11, 2026

Uh oh!

jorgemanrubia Jun 10, 2026

Uh oh!

samuelpecher left a comment

Uh oh!

samuelpecher Jun 11, 2026

Uh oh!

samuelpecher Jun 11, 2026

Uh oh!

samuelpecher Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// $findOrCreateGalleryForImage returns null when the resolved node can't join a gallery
		if (!this.#gallery) {

Conversation

jorgemanrubia commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

jorgemanrubia Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

samuelpecher Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

jorgemanrubia Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

samuelpecher left a comment

Choose a reason for hiding this comment

Uh oh!

samuelpecher Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

samuelpecher Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

samuelpecher Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jorgemanrubia commented Jun 10, 2026 •

edited

Loading