From 306311422c8a9b62c2045b99b0dcc6288a7b63aa Mon Sep 17 00:00:00 2001 From: ihordubas99 Date: Wed, 13 May 2026 17:18:25 +0300 Subject: [PATCH 1/2] fix: remove stuck placeholder block on failed attachment upload --- .../app/core/config/configuration.service.ts | 4 ++ .../hal/resources/configuration-resource.ts | 1 + .../react/components/OpBlockNoteEditor.tsx | 18 ++++- .../react/hooks/useAttachmentValidation.ts | 70 +++++++++++++++++++ .../react/hooks/useBlockNoteAttachments.ts | 49 +++++++++++-- .../configuration_representer.rb | 7 ++ 6 files changed, 140 insertions(+), 9 deletions(-) create mode 100644 frontend/src/react/hooks/useAttachmentValidation.ts diff --git a/frontend/src/app/core/config/configuration.service.ts b/frontend/src/app/core/config/configuration.service.ts index ee186227657a..e0f0862197d2 100644 --- a/frontend/src/app/core/config/configuration.service.ts +++ b/frontend/src/app/core/config/configuration.service.ts @@ -100,6 +100,10 @@ export class ConfigurationService { return this.systemPreference('allowedLinkProtocols') || null; } + public get attachmentWhitelist():string[] { + return this.systemPreference('attachmentWhitelist') ?? []; + } + public dateFormatPresent():boolean { return !!this.systemPreference('dateFormat'); } diff --git a/frontend/src/app/features/hal/resources/configuration-resource.ts b/frontend/src/app/features/hal/resources/configuration-resource.ts index 39b8547c926e..5fd070799591 100644 --- a/frontend/src/app/features/hal/resources/configuration-resource.ts +++ b/frontend/src/app/features/hal/resources/configuration-resource.ts @@ -31,4 +31,5 @@ import { HalResource } from 'core-app/features/hal/resources/hal-resource'; export class ConfigurationResource extends HalResource { public perPageOptions:number[]; public allowedLinkProtocols?:string[]; + public attachmentWhitelist?:string[]; } diff --git a/frontend/src/react/components/OpBlockNoteEditor.tsx b/frontend/src/react/components/OpBlockNoteEditor.tsx index ac3148f355ce..00d4fde5324b 100644 --- a/frontend/src/react/components/OpBlockNoteEditor.tsx +++ b/frontend/src/react/components/OpBlockNoteEditor.tsx @@ -36,7 +36,7 @@ import { BlockNoteView } from '@blocknote/mantine'; import { getDefaultReactSlashMenuItems, SuggestionMenuController, useCreateBlockNote } from '@blocknote/react'; import { HocuspocusProvider } from '@hocuspocus/provider'; import { initializeOpBlockNoteExtensions, openProjectWorkPackageBlockSpec, openProjectWorkPackageSlashMenu } from 'op-blocknote-extensions'; -import { useCallback, useEffect, useMemo } from 'react'; +import { useCallback, useEffect, useMemo, useRef } from 'react'; import * as Y from 'yjs'; import { useBlockNoteAttachments } from '../hooks/useBlockNoteAttachments'; import { useBlockNoteLocale } from '../hooks/useBlockNoteLocale'; @@ -79,7 +79,19 @@ export function OpBlockNoteEditor({ doc, }:OpBlockNoteEditorProps) { const { localeString, localeDictionary } = useBlockNoteLocale(window.I18n.locale); - const { enabled: attachmentsEnabled, uploadFile } = useBlockNoteAttachments(attachmentsCollectionKey, attachmentsUploadUrl); + + // useBlockNoteAttachments needs the editor instance to remove a stuck + // placeholder block on failed uploads, but the editor is created later + // in this function. We pass a lazy getter that reads from a ref assigned + // after useCreateBlockNote, breaking the would-be circular dependency. + const editorRef = useRef | null>(null); + const getEditor = useCallback(() => editorRef.current, []); + + const { enabled: attachmentsEnabled, uploadFile } = useBlockNoteAttachments( + attachmentsCollectionKey, + attachmentsUploadUrl, + getEditor, + ); useEffect(() => { initializeOpBlockNoteExtensions({ baseUrl: openProjectUrl, locale: localeString }); @@ -113,6 +125,8 @@ export function OpBlockNoteEditor({ }, [hocuspocusProvider, doc, activeUser, localeDictionary, attachmentsEnabled, uploadFile, captureExternalLinks]); const editor = useCreateBlockNote(editorParams, [activeUser]); + editorRef.current = editor; + type EditorType = typeof editor; const theme = useOpTheme(); diff --git a/frontend/src/react/hooks/useAttachmentValidation.ts b/frontend/src/react/hooks/useAttachmentValidation.ts new file mode 100644 index 000000000000..bdbbeafbdb8c --- /dev/null +++ b/frontend/src/react/hooks/useAttachmentValidation.ts @@ -0,0 +1,70 @@ +/* + * -- copyright + * OpenProject is an open source project management software. + * Copyright (C) the OpenProject GmbH + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License version 3. + * + * OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: + * Copyright (C) 2006-2013 Jean-Philippe Lang + * Copyright (C) 2010-2013 the ChiliProject Team + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + * + * See COPYRIGHT and LICENSE files for more details. + * ++ + */ + +import { useCallback } from 'react'; + +export interface AttachmentValidationResult { + valid:boolean; + reason?:string; +} + +export function useAttachmentValidation() { + const validateFile = useCallback(async (file:File):Promise => { + 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 }; + } + + return { + valid: false, + reason: window.I18n.t( + 'js.activerecord.errors.models.attachment.attributes.content_type.not_allowlisted', + { value: file.type }, + ), + }; + }, []); + + return { validateFile }; +} + +export default useAttachmentValidation; diff --git a/frontend/src/react/hooks/useBlockNoteAttachments.ts b/frontend/src/react/hooks/useBlockNoteAttachments.ts index 065da67dc6cb..7e2f2176ad23 100644 --- a/frontend/src/react/hooks/useBlockNoteAttachments.ts +++ b/frontend/src/react/hooks/useBlockNoteAttachments.ts @@ -31,15 +31,18 @@ import { IUploadFile } from 'core-app/core/upload/upload.service'; import { useCallback } from 'react'; import { firstValueFrom } from 'rxjs'; +import type { BlockNoteEditor } from '@blocknote/core'; +import { useAttachmentValidation } from './useAttachmentValidation'; export interface BlockNoteAttachmentsResult { enabled:boolean; - uploadFile?:(file:File) => Promise; + uploadFile?:(file:File, blockId?:string) => Promise; } export function useBlockNoteAttachments( attachmentsCollectionKey:string, attachmentsUploadUrl:string, + getEditor?:() => BlockNoteEditor | null, ):BlockNoteAttachmentsResult { const enabled = ( attachmentsCollectionKey !== undefined && @@ -48,25 +51,57 @@ export function useBlockNoteAttachments( attachmentsUploadUrl !== '' ); - const uploadFile = useCallback(async (file:File):Promise => { + const { validateFile } = useAttachmentValidation(); + + // BlockNote 0.44.x creates a "Loading..." placeholder block before awaiting + // uploadFile (blocknote.js Ie(), ~line 1130) and only calls updateBlock on + // the success path - it has no try/catch around the await. On rejection, + // the placeholder is left in the document forever. We remove it ourselves + // here. removeBlocks is synchronous and safe because BlockNote never + // touches the block again after the rejected await. + const removePlaceholder = useCallback((blockId?:string) => { + const editor = getEditor?.(); + if (!editor || !blockId) return; + try { + editor.removeBlocks([blockId]); + } catch { /* already removed by a collaborator via Yjs */ } + }, [getEditor]); + + const uploadFile = useCallback(async (file:File, blockId?:string):Promise => { const pluginContext = await window.OpenProject.getPluginContext(); + + const validation = await validateFile(file); + if (!validation.valid) { + pluginContext.services.notifications.addError(validation.reason ?? 'File not allowed'); + removePlaceholder(blockId); + return ''; + } + try { const service = pluginContext.services.attachmentsResourceService; const uploadFiles:IUploadFile[] = [{ file }]; const result = await firstValueFrom( - service.addAttachments(attachmentsCollectionKey, attachmentsUploadUrl, uploadFiles) + service.addAttachments(attachmentsCollectionKey, attachmentsUploadUrl, uploadFiles), ); - return result?.[0]._links.staticDownloadLocation.href ?? ''; + const href = result?.[0]?._links?.staticDownloadLocation?.href; + if (!href) { + throw new Error('Upload returned no download location'); + } + return href; // eslint-disable-next-line @typescript-eslint/no-explicit-any } catch (error:any) { - const toastService = pluginContext.services.notifications; // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - toastService.addError(error); + pluginContext.services.notifications.addError(error); + removePlaceholder(blockId); + // Return '' instead of rethrowing: BlockNote 0.44.x doesn't catch + // uploadFile rejections, so throwing would surface as Uncaught (in + // promise). The placeholder is already gone, so the success-path + // updateBlock that follows our return won't fire anyway. return ''; } - }, [attachmentsCollectionKey, attachmentsUploadUrl]); + }, [attachmentsCollectionKey, attachmentsUploadUrl, validateFile, removePlaceholder]); if (!enabled) { return { enabled }; diff --git a/lib/api/v3/configuration/configuration_representer.rb b/lib/api/v3/configuration/configuration_representer.rb index a708e87e3f4f..e686453144dd 100644 --- a/lib/api/v3/configuration/configuration_representer.rb +++ b/lib/api/v3/configuration/configuration_representer.rb @@ -116,6 +116,13 @@ class ConfigurationRepresenter < ::API::Decorators::Single property :allowed_link_protocols, getter: ->(*) { Setting::AllowedLinkProtocols.all } + property :attachment_whitelist, + getter: ->(*) { + raw = Setting.attachment_whitelist + Array(raw).flat_map { |entry| entry.to_s.split(/\r?\n/) } + .map(&:strip) + .reject(&:blank?) + } property :user_preferences, embedded: true, exec_context: :decorator, From 0cf036f4f8235aff0ecb4a0cfad84645f08007e3 Mon Sep 17 00:00:00 2001 From: ihordubas99 Date: Thu, 14 May 2026 12:00:20 +0300 Subject: [PATCH 2/2] fix: clean up attachment upload error handling and lint issues --- config/locales/js-en.yml | 1 + frontend/src/react/components/OpBlockNoteEditor.tsx | 4 ++-- frontend/src/react/hooks/useAttachmentValidation.ts | 2 +- frontend/src/react/hooks/useBlockNoteAttachments.ts | 7 +++++-- lib/api/v3/configuration/configuration_representer.rb | 9 +++++---- 5 files changed, 14 insertions(+), 9 deletions(-) diff --git a/config/locales/js-en.yml b/config/locales/js-en.yml index 50545ecb1255..7ddfd8ebe1b2 100644 --- a/config/locales/js-en.yml +++ b/config/locales/js-en.yml @@ -1078,6 +1078,7 @@ en: error_could_not_resolve_user_name: "Couldn't resolve user name" error_attachment_upload: "File failed to upload: %{error}" error_attachment_upload_permission: "You don't have the permission to upload files on this resource." + error_attachment_type_not_allowed: "The file was rejected by an automatic filter. '%{value}' is not allowed for upload." units: workPackage: diff --git a/frontend/src/react/components/OpBlockNoteEditor.tsx b/frontend/src/react/components/OpBlockNoteEditor.tsx index 00d4fde5324b..35764a2f9e39 100644 --- a/frontend/src/react/components/OpBlockNoteEditor.tsx +++ b/frontend/src/react/components/OpBlockNoteEditor.tsx @@ -38,7 +38,7 @@ import { HocuspocusProvider } from '@hocuspocus/provider'; import { initializeOpBlockNoteExtensions, openProjectWorkPackageBlockSpec, openProjectWorkPackageSlashMenu } from 'op-blocknote-extensions'; import { useCallback, useEffect, useMemo, useRef } from 'react'; import * as Y from 'yjs'; -import { useBlockNoteAttachments } from '../hooks/useBlockNoteAttachments'; +import { useBlockNoteAttachments, BlockNoteEditorRef } from '../hooks/useBlockNoteAttachments'; import { useBlockNoteLocale } from '../hooks/useBlockNoteLocale'; import { useOpTheme } from '../hooks/useOpTheme'; @@ -84,7 +84,7 @@ export function OpBlockNoteEditor({ // placeholder block on failed uploads, but the editor is created later // in this function. We pass a lazy getter that reads from a ref assigned // after useCreateBlockNote, breaking the would-be circular dependency. - const editorRef = useRef | null>(null); + const editorRef = useRef(null); const getEditor = useCallback(() => editorRef.current, []); const { enabled: attachmentsEnabled, uploadFile } = useBlockNoteAttachments( diff --git a/frontend/src/react/hooks/useAttachmentValidation.ts b/frontend/src/react/hooks/useAttachmentValidation.ts index bdbbeafbdb8c..9446288ac6df 100644 --- a/frontend/src/react/hooks/useAttachmentValidation.ts +++ b/frontend/src/react/hooks/useAttachmentValidation.ts @@ -58,7 +58,7 @@ export function useAttachmentValidation() { return { valid: false, reason: window.I18n.t( - 'js.activerecord.errors.models.attachment.attributes.content_type.not_allowlisted', + 'js.error_attachment_type_not_allowed', { value: file.type }, ), }; diff --git a/frontend/src/react/hooks/useBlockNoteAttachments.ts b/frontend/src/react/hooks/useBlockNoteAttachments.ts index 7e2f2176ad23..41de30a34e7b 100644 --- a/frontend/src/react/hooks/useBlockNoteAttachments.ts +++ b/frontend/src/react/hooks/useBlockNoteAttachments.ts @@ -31,7 +31,6 @@ import { IUploadFile } from 'core-app/core/upload/upload.service'; import { useCallback } from 'react'; import { firstValueFrom } from 'rxjs'; -import type { BlockNoteEditor } from '@blocknote/core'; import { useAttachmentValidation } from './useAttachmentValidation'; export interface BlockNoteAttachmentsResult { @@ -39,10 +38,14 @@ export interface BlockNoteAttachmentsResult { uploadFile?:(file:File, blockId?:string) => Promise; } +export interface BlockNoteEditorRef { + removeBlocks:(ids:string[]) => void; +} + export function useBlockNoteAttachments( attachmentsCollectionKey:string, attachmentsUploadUrl:string, - getEditor?:() => BlockNoteEditor | null, + getEditor?:() => BlockNoteEditorRef | null, ):BlockNoteAttachmentsResult { const enabled = ( attachmentsCollectionKey !== undefined && diff --git a/lib/api/v3/configuration/configuration_representer.rb b/lib/api/v3/configuration/configuration_representer.rb index e686453144dd..9dafe559c82d 100644 --- a/lib/api/v3/configuration/configuration_representer.rb +++ b/lib/api/v3/configuration/configuration_representer.rb @@ -118,11 +118,12 @@ class ConfigurationRepresenter < ::API::Decorators::Single property :attachment_whitelist, getter: ->(*) { - raw = Setting.attachment_whitelist - Array(raw).flat_map { |entry| entry.to_s.split(/\r?\n/) } - .map(&:strip) - .reject(&:blank?) + raw = Setting.attachment_whitelist + Array(raw).flat_map { |entry| entry.to_s.split(/\r?\n/) } + .map(&:strip) + .compact_blank } + property :user_preferences, embedded: true, exec_context: :decorator,