[COMMS-510] Remove the stuck placeholder of a failed file drop or paste by ihordubas99 · Pull Request #23199 · opf/openproject

ihordubas99 · 2026-05-13T14:25:24Z

Ticket

https://community.openproject.org/wp/COMMS-510

What are you trying to accomplish?

When a file dropped or pasted into a document is rejected, for example because of the attachment allowlist, the error toast appears but a "Loading..." block stays in the document and has to be deleted by hand. After this change, only the error remains.

What approach did you choose and why?

The cause is in BlockNote, still in 0.55. A file block swaps its "Add file" button for "Loading..." when the upload starts, and only renders again when its props change. After a failed drop or paste BlockNote sets the same empty url again, so the block never renders again.

A small BlockNote extension, FailedUploadCleanupExtension, listens for the end of each upload. If the block still shows "Loading..." once BlockNote has updated it, the extension removes it. The check runs in a timeout because BlockNote updates the block only after the upload ends.

A failed upload from the file panel (/image) is left alone. There BlockNote sets the file name, the block renders again and offers "Add image", so the reader can retry.

Changes since the last review

The PR is rewritten on top of the current dev and is much smaller now.

The placeholder is no longer removed inside uploadFile. BlockNote always calls updateBlock after uploadFile returns, so removing the block there made every failed upload end in an unhandled Block with ID … not found error. It also removed the block of a failed file panel upload, which on dev offers to add a file again.
The client-side allowlist check is gone, and with it the attachmentWhitelist configuration property, its API docs and specs. The backend matches the allowlist against the content type it detects itself, which can differ from the browser's file.type, so the two checks could disagree in either direction. The backend error toast already tells the reader what went wrong.

Tests

failed-upload-cleanup.spec.ts runs a real BlockNote editor in Chromium. It covers a failed drop, a failed paste, a successful drop, a failed file panel upload and unmounting during an upload.
attachment_upload_spec.rb gains a scenario that drops a file the allowlist rejects and fails without the fix. It uses a new drop_file helper, because Capybara's drop sends no coordinates and BlockNote ignores such a drop.

Reported upstream as TypeCellOS/BlockNote#3246

AI involvement

Collaborative – AI generated a substantial part of the code; I reviewed and understand every line.

Merge checklist

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

github-actions · 2026-05-14T15:59:47Z

Caution

The provided work package version does not match the core version

Details:

Work package URL: https://community.openproject.org/wp/COMMS-510
Work package version: Product Backlog
Core version: 18.0.0

Please make sure that:

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

akabiru

preliminary scan with review agent

akabiru · 2026-05-20T18:40:55Z

+      return { valid: true };
+    }
+
+    if (whitelist.includes(file.type)) {


🟠 Should-fix — client-side check doesn't honour the *ext whitelist form.

The backend allowlist accepts two entry shapes: bare MIME types (image/png) and extension globs (*png). See app/contracts/attachments/create_contract.rb:95:

allowlist.include?(model.content_type) || allowlist.include?("*#{model.extension}")

whitelist.includes(file.type) only matches the MIME form. An admin who configured the whitelist as *png\n*jpg (the form shown in the docs) will see legitimate PNG drops rejected client-side even though the backend would accept them — a regression dressed up as a feature.

Split the check:

const ext = file.name.split('.').pop()?.toLowerCase(); const allowed = whitelist.some((entry) => entry.startsWith('*') ? entry.slice(1).toLowerCase() === ext : entry === file.type, );

Or the conservative variant: only fast-fail on bare-MIME entries and defer everything else to the backend, same pattern as the empty-file.type branch above.

Fixed - updated the check to handle both *.png and *png glob formats by stripping * and optional leading . before comparing against the file extension.

akabiru · 2026-05-20T18:40:55Z

  }, [hocuspocusProvider, doc, activeUser, localeDictionary, attachmentsEnabled, uploadFile, captureExternalLinks]);

  const editor = useCreateBlockNote(editorParams, [activeUser]);
+  editorRef.current = editor;


🟠 Should-fix — ref assigned during render.

Mutating a ref in the render body is a React anti-pattern: eslint-plugin-react-hooks flags it, and under <StrictMode> the render runs twice so the assignment happens twice (harmless here, but a smell). Move it into an effect:

useEffect(() => { editorRef.current = editor; }, [editor]);

getEditor is only invoked from the upload-rejection path, which fires after mount, so the one-tick delay between render and effect is safe.

Fixed - moved editorRef.current = editor into a useEffect

akabiru · 2026-05-20T18:40:55Z

+  uploadFile?:(file:File, blockId?:string) => Promise<string>;
+}
+
+export interface BlockNoteEditorRef {


🟢 Nit — interface name oversells its scope.

BlockNoteEditorRef reads as "a reference to the whole editor" but the shape only carries removeBlocks. Renaming to something like PlaceholderRemover (or inlining the type at the single usage point and dropping the interface) makes the responsibility match the API surface.

Renamed BlockNoteEditorRef -> EditorHandle

akabiru · 2026-05-20T18:40:55Z

+      }
+      return href;
      // eslint-disable-next-line @typescript-eslint/no-explicit-any
    } catch (error:any) {


🟢 Nit — two eslint-disable lines can go.

catch (error) infers unknown in TS 4.4+, and the toast service accepts strings — narrow at the callsite instead of typing as any:

} catch (error) { pluginContext.services.notifications.addError( error instanceof Error ? error.message : String(error), ); removePlaceholder(blockId); return ''; }

Both disable comments fall out.

akabiru · 2026-05-20T18:40:55Z


+        property :attachment_whitelist,
+                 getter: ->(*) {
+                   raw = Setting.attachment_whitelist


🟢 Nit — duplicates the controller's normalisation.

app/controllers/admin/settings/attachments_settings_controller.rb:37 already splits, strips, and compacts on write, so the persisted value is a clean array. If that's the only write path the getter can shrink to:

getter: ->(*) { Array(Setting.attachment_whitelist) }

If there's a reason to keep the defensive parse (e.g. settings imported via a different path), a one-line comment naming that reason would carry its weight.

🍊 We will also need to update open-api documentation for this new value -> https://github.com/opf/openproject/blob/dev/docs/api/apiv3/components/schemas/configuration_model.yml

🍊 and a unit test in spec/requests/api/v3/configuration_resource_spec.rb

Simplified to Array(Setting.attachment_whitelist).map(&:strip).compact_blank - the flat_map/split is no longer needed since the controller already persists a clean array. Kept strip + compact_blank as defensive normalization since the controller doesn't strip on write.

Added attachmentWhitelist to docs/api/apiv3/components/schemas/configuration_model.yml.

Added two tests - with a configured whitelist and with an empty whitelist.

akabiru · 2026-05-20T18:40:55Z

+  public get attachmentWhitelist():string[] {
+    return this.systemPreference<string[]>('attachmentWhitelist') ?? [];
+  }
+


🟢 Nit — trailing whitespace on this line; eslint / editorconfig will flag.

Copilot

Pull request overview

Fixes a BlockNote editor UX bug where failed attachment uploads (e.g., disallowed file types) leave a permanent “Loading…” placeholder block, and adds client-side pre-validation based on the server’s attachment allowlist exposed via the existing /api/v3/configuration endpoint.

Changes:

Expose Setting.attachment_whitelist via APIv3 configuration and consume it in the frontend configuration service.
Update BlockNote upload integration to remove the placeholder block when uploadFile fails or client-side validation rejects the file.
Add a React hook to validate dropped files against the attachment allowlist and show a localized error message.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
lib/api/v3/configuration/configuration_representer.rb	Adds `attachmentWhitelist` to configuration payload, normalizing Setting values into a string array.
frontend/src/app/features/hal/resources/configuration-resource.ts	Extends the HAL configuration resource with `attachmentWhitelist`.
frontend/src/app/core/config/configuration.service.ts	Adds a typed getter to read `attachmentWhitelist` from the configuration resource.
frontend/src/react/hooks/useAttachmentValidation.ts	Introduces client-side attachment MIME allowlist validation and localized error reason.
frontend/src/react/hooks/useBlockNoteAttachments.ts	Adds BlockNote placeholder cleanup on validation failure / upload rejection, and integrates validation before upload.
frontend/src/react/components/OpBlockNoteEditor.tsx	Passes a lazy editor getter into attachments hook so it can remove placeholder blocks.
config/locales/js-en.yml	Adds a new localized JS error string for disallowed attachment types.

+    const pluginContext = await window.OpenProject.getPluginContext();
+    const whitelist = pluginContext.services.configurationService.attachmentWhitelist;
+
+    if (!whitelist || whitelist.length === 0) {
+      return { valid: true };
+    }
+
+    // Empty file.type means the browser couldn't infer the MIME from the
+    // extension (e.g. .xyz). Defer to the backend in that case - it does
+    // real magic-byte detection that we can't replicate cheaply on the client.
+    if (!file.type) {
+      return { valid: true };
+    }
+
+    if (whitelist.includes(file.type)) {
+      return { valid: true };
+    }


+        property :attachment_whitelist,
+                 getter: ->(*) {
+                   raw = Setting.attachment_whitelist
+                   Array(raw).flat_map { |entry| entry.to_s.split(/\r?\n/) }
+                             .map(&:strip)
+                             .compact_blank
+                 }


akabiru · 2026-05-21T13:57:27Z

@ihordubas99 as of #23207 we now have the ability to unit test react hooks- shall we add some coverage here?

akabiru · 2026-05-21T14:20:11Z

+
+    const validation = await validateFile(file);
+    if (!validation.valid) {
+      pluginContext.services.notifications.addError(validation.reason ?? 'File not allowed');


🍊 We should probably be using translations here- OP is multi-lingual. See frontend/src/app/core/i18n/i18n.service.ts

Better yet - see how useBlockNoteLocale works via window.i18n.locale

Replaced the reason?: string interface with a discriminated union type - { valid: true } | { valid: false; reason: string }. This guarantees reason is always a string when valid is false, so the hardcoded 'File not allowed' fallback is no longer needed. The reason is already translated via window.I18n.t

Copilot

Pull request overview

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

+        property :attachment_whitelist,
+                 getter: ->(*) {
+                   Array(Setting.attachment_whitelist).map(&:strip).compact_blank
+                 }


+      if (!href) {
+        throw new Error('Upload returned no download location');
+      }


+    } catch (error) {
+      pluginContext.services.notifications.addError(
+        error instanceof Error ? error.message : String(error),
+      );
+      removePlaceholder(blockId);


Copilot

Pull request overview

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

+  const uploadFile = useCallback(async (file:File, blockId?:string):Promise<string> => {
    const pluginContext = await window.OpenProject.getPluginContext();
+
+    const validation = await validateFile(file);
+    if (!validation.valid) {
+      pluginContext.services.notifications.addError(validation.reason);
+      removePlaceholder(blockId);
+      return '';
+    }
+
    try {


+      pluginContext.services.notifications.addError(
+        error instanceof Error ? error.message : String(error),
+      );


+    const ext = file.name.split('.').pop()?.toLowerCase();
+    const allowed = whitelist.some((entry) =>
+      entry.startsWith('*')
+        ? entry.slice(1).replace(/^\./, '').toLowerCase() === ext
+        : entry === file.type,


+function mockFile(name:string, type:string):File {
+  return new File([''], name, { type });
+}
+
+beforeEach(() => {
+  (window as unknown as { I18n:unknown }).I18n = { t: (_key:string, opts?:{ value?:string }) => `not allowed: ${opts?.value ?? ''}` };
+});


Copilot

Pull request overview

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

+      return href;
+    } catch (error) {
+      const pluginContext = await window.OpenProject.getPluginContext().catch(() => null);
+      pluginContext?.services.notifications.addError(error as never);


+    const ext = file.name.split('.').pop()?.toLowerCase();
+    const allowed = whitelist.some((entry) =>
+      entry.startsWith('*.')
+        ? entry.slice(2).toLowerCase() === ext
+        : entry === file.type,
+    );


+    it('is case-insensitive for the extension', async () => {
+      mockPluginContext(['*.PNG']);
+      const { result } = renderHook(() => useAttachmentValidation());
+      const validation = await result.current.validateFile(mockFile('photo.png', 'image/png'));
+      expect(validation.valid).toBe(true);
+    });


ihordubas99 · 2026-10-23T13:25:37Z

Hi @akabiru, sorry, I lost track of this PR. I went through it all again and realised my earlier approach was wrong at its core. The root cause is in BlockNote itself, and I've reported it to them as TypeCellOS/BlockNote#3246. Until it's fixed there, I've replaced the PR with a much smaller fix, and the description explains what changed and why. If you find the time, I'd appreciate another look. Thanks!

github-actions · 2026-10-23T13:49:45Z

Warning

Flaky specs

rspec ./spec/features/work_packages/table/queries/id_filter_spec.rb[1:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #23199, linked for reference only):

- `rspec ./spec/features/work_packages/table/queries/id_filter_spec.rb[1:1]`

Treat this as a standalone task, unrelated to PR #23199. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #23199 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @ihordubas99 to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @ihordubas99, and request a review from @ihordubas99.
On every commit, set @ihordubas99 as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

nperez0111 · 2026-10-23T14:36:36Z

I'll get a fix for this in BlockNote

ihordubas99 · 2026-10-27T13:51:26Z

Thanks a lot @nperez0111 for the quick fix in TypeCellOS/BlockNote#3247!

The fix is merged upstream but not released yet. Once BlockNote ships a release that includes it, I'll bump BlockNote in OpenProject and close this PR.

nperez0111 · 2026-10-27T14:03:01Z

👍 for sure. Hopefully this week

ihordubas99 self-assigned this May 13, 2026

ihordubas99 force-pushed the bug/68792-blocknote-loading-persists-on-unsupported-file branch from cef11fa to 0cf036f Compare May 14, 2026 09:15

ihordubas99 marked this pull request as ready for review May 14, 2026 15:59

ihordubas99 requested a review from akabiru May 15, 2026 11:29

akabiru requested a review from Copilot May 21, 2026 13:52

Copilot started reviewing on behalf of akabiru May 21, 2026 13:53

View session

akabiru reviewed May 21, 2026

View reviewed changes

Copilot AI reviewed May 21, 2026

View reviewed changes

akabiru requested changes May 21, 2026

View reviewed changes

akabiru reviewed May 21, 2026

View reviewed changes

ihordubas99 force-pushed the bug/68792-blocknote-loading-persists-on-unsupported-file branch from 5f14e80 to 865529c Compare June 2, 2026 14:09

ihordubas99 requested a review from Copilot June 2, 2026 14:10

Copilot started reviewing on behalf of ihordubas99 June 2, 2026 14:10

View session

ihordubas99 marked this pull request as draft June 2, 2026 14:10

Copilot AI reviewed Jun 2, 2026

View reviewed changes

ihordubas99 force-pushed the bug/68792-blocknote-loading-persists-on-unsupported-file branch from 865529c to 420e572 Compare June 3, 2026 13:48

ihordubas99 requested a review from Copilot June 3, 2026 14:29

Copilot started reviewing on behalf of ihordubas99 June 3, 2026 14:29

View session

Copilot AI reviewed Jun 3, 2026

View reviewed changes

ihordubas99 marked this pull request as ready for review June 3, 2026 15:03

ihordubas99 requested review from akabiru and Copilot June 3, 2026 15:04

Copilot started reviewing on behalf of ihordubas99 June 3, 2026 15:04

View session

ihordubas99 marked this pull request as draft June 3, 2026 15:09

Copilot AI reviewed Jun 3, 2026

View reviewed changes

ihordubas99 marked this pull request as ready for review June 4, 2026 11:26

ihordubas99 force-pushed the bug/68792-blocknote-loading-persists-on-unsupported-file branch from e1efd32 to a0c7e76 Compare October 23, 2026 12:43

ihordubas99 changed the title ~~fix: remove stuck placeholder block on failed attachment upload~~ [COMMS-510] Remove the stuck placeholder of a failed file drop or paste Oct 23, 2026

github-actions Bot added the ai: Collaborative 💻 AI generated a substantial part of the code; A human reviewed and understands every line. label Oct 23, 2026

[COMMS-510] Remove the stuck placeholder of a failed file drop or paste

cb8a834

ihordubas99 force-pushed the bug/68792-blocknote-loading-persists-on-unsupported-file branch from a0c7e76 to cb8a834 Compare October 23, 2026 13:24

Conversation

ihordubas99 commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

What approach did you choose and why?

Changes since the last review

Tests

AI involvement

Merge checklist

Uh oh!

github-actions Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akabiru left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akabiru May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

akabiru commented May 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

ihordubas99 commented Oct 23, 2026

Uh oh!

github-actions Bot commented Oct 23, 2026

Uh oh!

nperez0111 commented Oct 23, 2026

Uh oh!

ihordubas99 commented Oct 27, 2026

Uh oh!

nperez0111 commented Oct 27, 2026

Uh oh!

ihordubas99 commented May 13, 2026 •

edited

Loading

github-actions Bot commented May 14, 2026 •

edited

Loading

akabiru May 21, 2026 •

edited

Loading