fix: remove stuck placeholder block on failed attachment upload#23199
Open
ihordubas99 wants to merge 2 commits into
Open
fix: remove stuck placeholder block on failed attachment upload#23199ihordubas99 wants to merge 2 commits into
ihordubas99 wants to merge 2 commits into
Conversation
cef11fa to
0cf036f
Compare
|
Caution The provided work package version does not match the core version Details:
Please make sure that:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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