Skip to content

fix: remove stuck placeholder block on failed attachment upload#23199

Open
ihordubas99 wants to merge 2 commits into
devfrom
bug/68792-blocknote-loading-persists-on-unsupported-file
Open

fix: remove stuck placeholder block on failed attachment upload#23199
ihordubas99 wants to merge 2 commits into
devfrom
bug/68792-blocknote-loading-persists-on-unsupported-file

Conversation

@ihordubas99
Copy link
Copy Markdown
Collaborator

Ticket

https://community.openproject.org/projects/communicator-stream/work_packages/68792

What are you trying to accomplish?

Closes the bug where dragging a file with an unsupported extension into a BlockNote editor leaves a "Loading..." placeholder block stuck in the document. The backend rejects the upload and shows the error toast, but BlockNote never cleans up the placeholder, forcing the user to click and delete it manually.
In addition to fixing the visible bug, this PR adds client-side pre-validation of the file's MIME type so that obviously disallowed files (e.g. dropping .txt when the whitelist is [image/png]) are rejected instantly without an upload round-trip.

What approach did you choose and why?

The bug lives in BlockNote 0.44.2, not in our code. Its drop handler (Ie() around blocknote.js:1130) creates a placeholder block, then does await editor.uploadFile(file, blockId) with no try/catch, and calls updateBlock only on the success path. On rejection, the placeholder is orphaned and there is no hook to clean it up upstream.
Three options were on the table:

Patch BlockNote (upstream PR or local patch-package). Cleanest semantically, but slow and adds maintenance burden until merged.
Workaround in our uploadFile. Catch rejections inside the function we already pass to BlockNote and remove the placeholder ourselves.
Pre-validate before the file ever reaches uploadFile. Through a ProseMirror plugin that intercepts drop/paste earlier than BlockNote.

I went with option 2 as the primary fix (it fully closes the ticket) and layered a lightweight version of option 3 on top of it (MIME pre-validation inside uploadFile, before the upload starts). The pure ProseMirror-plugin variant was rejected because making sure our handler runs before BlockNote's would be brittle and the UX win over "placeholder appears for ~50ms then disappears" is marginal.
Implementation details:

useBlockNoteAttachments accepts a lazy getEditor getter so it can call editor.removeBlocks([blockId]) from its rejection path without creating a circular dependency with useCreateBlockNote. The ref is read only inside the catch (always after the editor is assigned), so there is no race.
removeBlocks is called synchronously in the catch - verified by reading blocknote.js: the updateBlock call sits after the awaited uploadFile, so on rejection BlockNote never touches the block again. No queueMicrotask or other deferral hack is needed.
uploadFile returns '' instead of rethrowing on failure. Rethrowing would surface as Uncaught (in promise) because BlockNote 0.44.2 does not catch its own await uploadFile. Since the placeholder has already been removed by the time we return, the success-path updateBlock that follows our return is harmless on rejection (it doesn't run) and a no-op concern on the validation path either.
Pre-validation lives in a separate useAttachmentValidation hook. It reads attachment_whitelist from ConfigurationService, which I extended along with ConfigurationResource and ConfigurationRepresenter to expose Setting.attachment_whitelist through the existing /api/v3/configuration endpoint - the same channel already used for attachment_max_size, allowed_link_protocols, etc. No new endpoint, no new config mechanism.
Validation deliberately skips files with an empty file.type (e.g. .xyz). Browsers can't infer a MIME for unknown extensions, and blocking them client-side would also block legitimate cases. The backend does proper magic-byte detection via marcel, so we defer to it. These files still produce the cleanest possible UX because the placeholder cleanup from option 2 handles them.
The user-facing error string reuses the existing activerecord.errors.models.attachment.attributes.content_type.not_allowlisted translation so the message is identical whether the rejection comes from the client or the server.

Known limitations (documented in code comments):

The placeholder cleanup is tied to BlockNote 0.44.2 internals. If a future version starts catching uploadFile rejections itself, our removeBlocks will become a redundant call on an already-removed block - caught by the inner try/catch, but worth re-evaluating on upgrade. The version is named in the comment for that reason.
file.type from the browser is extension-based, not magic-byte based. A renamed evil.exe -> cute.png would pass our check and fail server-side. This is expected: client-side validation is for UX, server-side is for security.

Merge checklist

  • Added/updated tests
  • Added/updated documentation in Lookbook (patterns, previews, etc)
  • Tested major browsers (Chrome, Firefox, Edge, ...)

@ihordubas99 ihordubas99 self-assigned this May 13, 2026
@ihordubas99 ihordubas99 force-pushed the bug/68792-blocknote-loading-persists-on-unsupported-file branch from cef11fa to 0cf036f Compare May 14, 2026 09:15
@ihordubas99 ihordubas99 marked this pull request as ready for review May 14, 2026 15:59
@github-actions
Copy link
Copy Markdown

Caution

The provided work package version does not match the core version

Details:

Please make sure that:

  • The work package version OR your pull request target branch is correct

@ihordubas99 ihordubas99 requested a review from akabiru May 15, 2026 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant