feat: integrate Collabora using iframe [WPB-21650]

[e-maad]

WPB-21650 [Web] Collabora integration

Pull Request

Summary

We have to provide users a feature where they can collaborate on files. Like real time editing as a team.
With Collabora integration, we can allow user to achieve it.

Security Checklist (required)

• External inputs are validated & sanitized on client and/or server where applicable.
• API responses are validated; unexpected shapes are handled safely (fallbacks or errors).
• No unsafe HTML is rendered; if unavoidable, sanitization is applied and documented where it happens.
• Injection risks (XSS/SQL/command) are prevented via safe APIs and/or escaping.

Standards Acknowledgement (required)

• I have read and this PR upholds our Coding Standards and Tech Radar Choices.

Screenshots or demo (if the user interface changed)

Notes for reviewers

Screen.Recording.2025-11-28.at.16.07.26.mov

• Trade-offs:
  We are using <iframe> to render the URL as we don't have native integration for Collabora.
• Follow-ups (linked issues):
• Linked PRs (e.g. web-packages):

[Copilot]

Pull request overview

This PR integrates Collabora Online for real-time collaborative document editing via iframe embedding. Users can now edit supported file types (docx, xlsx, pptx, odf) directly within the Wire webapp through a toggle between "Viewing" and "Editing" modes in the file preview modal.

Key Changes:

• Added FileEditor component that renders Collabora URLs in an iframe with auto-refresh logic before URL expiry
• Extended file preview modals with edit mode state management and UI toggle buttons
• Implemented isFileEditable() utility to determine which file extensions support collaborative editing
• Propagated asset IDs (UUIDs) through component hierarchy replacing React useId() for consistent identification

Reviewed changes

Copilot reviewed 30 out of 31 changed files in this pull request and generated 19 comments.

Show a summary per file

File	Description
src/script/components/FileFullscreenModal/FileEditor/FileEditor.tsx	New component rendering Collabora iframe with URL refresh logic
src/script/components/FileFullscreenModal/FileEditor/FileEditor.styles.ts	Styles for the iframe editor (full width/height)
src/script/components/FileFullscreenModal/FileFullscreenModal.tsx	Added edit mode state and conditional rendering of FileEditor vs standard preview
src/script/components/FileFullscreenModal/FileHeader/FileHeader.tsx	Added Viewing/Editing toggle buttons and updated download logic to fetch fresh presigned URLs
src/script/components/FileFullscreenModal/FileHeader/FileHeader.styles.ts	Styles for edit mode toggle button group
src/script/util/FileTypeUtil.ts	New isFileEditable() function checking if file extension supports editing
src/script/util/FileTypeUtil.test.ts	Comprehensive tests for isFileEditable() covering various edge cases
src/script/util/getFileTypeFromExtension/getFileTypeFromExtension.ts	Added 'odf' to document extensions list
src/script/repositories/cells/CellsRepository.ts	Extended getNode() to accept flags and options parameters
src/script/components/MessagesList/Message/ContentMessage/asset/MultipartAssets/FileAssetCard/common/FileAssetOptions/FileAssetOptions.tsx	Added "Edit" option to file dropdown menu for editable files
src/script/components/MessagesList/Message/ContentMessage/asset/MultipartAssets/FileAssetCard/common/FileAssetOptions/FileAssetOptions.test.tsx	Tests verifying edit option behavior with different file types
src/script/components/MessagesList/Message/ContentMessage/asset/MultipartAssets/FileAssetCard/common/FilePreviewModal/FilePreviewModal.tsx	Pass through isEditMode prop
src/script/components/MessagesList/Message/ContentMessage/asset/MultipartAssets/FileAssetCard/FileAssetWithPreview/FileAssetWithPreview.tsx	Track edit mode state and handle modal open/close with edit flag
src/script/components/MessagesList/Message/ContentMessage/asset/MultipartAssets/FileAssetCard/FileAssetSmall/FileAssetSmall.tsx	Track edit mode state and pass through asset ID
src/script/components/MessagesList/Message/ContentMessage/asset/MultipartAssets/FileAssetCard/FileAssetCard.tsx	Pass through asset ID prop
src/script/components/MessagesList/Message/ContentMessage/asset/MultipartAssets/MultipartAssets.tsx	Pass asset UUID as ID to child components
src/script/components/MessagesList/Message/ContentMessage/asset/MultipartAssets/ImageAssetCard/*	Replace useId() with prop-based ID (UUID)
src/script/components/MessagesList/Message/ContentMessage/asset/MultipartAssets/VideoAssetCard/*	Replace useId() with prop-based ID (UUID)
src/script/components/Conversation/ConversationCells/CellsTable/common/CellsFilePreviewModalContext/CellsFilePreviewModalContext.tsx	Add isEditMode state and parameter to handleOpenFile
src/script/components/Conversation/ConversationCells/CellsTable/CellsTableColumns/CellsTableRowOptions/CellsTableRowOptions.tsx	Add "Edit" option for editable files in table row dropdown
src/script/components/Conversation/ConversationCells/CellsTable/CellsFilePreviewModal/CellsFilePreviewModal.tsx	Use selectedFile.id instead of context ID, pass isEditMode
src/script/components/CellsGlobalView/CellsTable/common/CellsFilePreviewModalContext/CellsFilePreviewModalContext.tsx	Add isEditMode state and parameter to handleOpenFile
src/script/components/CellsGlobalView/CellsTable/CellsTableColumns/CellsTableRowOptions/CellsTableRowOptions.tsx	Add "Edit" option for editable files in table row dropdown
src/script/components/CellsGlobalView/CellsTable/CellsFilePreviewModal/CellsFilePreviewModal.tsx	Use selectedFile.id instead of context ID, pass isEditMode
src/i18n/en-US.json	Added translation keys for edit option and editor error messages
src/types/i18n.d.ts	Type definitions for new i18n keys

[Copilot]

[Important] The handleFileDownload function lacks error handling. If the API call or download fails, the user will get no feedback.

Add try-catch with user-facing error handling:

const handleFileDownload = async () => {
  if (!fileUrl) return;
  
  try {
    const node = await cellsRepository.getNode({uuid: id, flags: []});
    await forcedDownloadFile({url: node.PreSignedGET?.Url || fileUrl, name: fileNameWithExtension});
  } catch (error) {
    // Show user-facing error notification
    console.error('Failed to download file:', error);
    // Consider adding a toast notification here
  }
};

Suggested change

	if (fileUrl) {
	const node = await cellsRepository.getNode({uuid: id, flags: []});
	await forcedDownloadFile({url: node.PreSignedGET?.Url || fileUrl, name: fileNameWithExtension});
	if (!fileUrl) return;
	try {
	const node = await cellsRepository.getNode({uuid: id, flags: []});
	await forcedDownloadFile({url: node.PreSignedGET?.Url || fileUrl, name: fileNameWithExtension});
	} catch (error) {
	// Show user-facing error notification
	console.error('Failed to download file:', error);
	// If a toast notification system is available, use it:
	// toast.error('Failed to download file. Please try again.');
	// Otherwise, fallback to alert:
	alert('Failed to download file. Please try again.');

[Copilot]

[Important] The new FileEditor component lacks test coverage. Given its security-sensitive nature (rendering iframes with external URLs), it should have comprehensive tests covering:

• URL validation scenarios
• Error states
• Loading states
• Timeout refresh logic
• Edge cases with expiry times

Consider adding tests in a new FileEditor.test.tsx file.

[Copilot]

[Blocker] The timeout calculation can create negative or zero timeouts when expiresInSeconds is less than or equal to REFRESH_BUFFER_SECONDS (10 seconds). This will cause immediate re-fetching in a loop.

Add validation:

const expiresInSeconds = Number(node.EditorURLs.collabora.ExpiresAt);
const refreshInSeconds = Math.max(0, expiresInSeconds - REFRESH_BUFFER_SECONDS);

// Only set timeout if there's meaningful time left
if (refreshInSeconds > 0) {
  const timeoutId = setTimeout(() => {
    void fetchNode();
  }, refreshInSeconds * MILLISECONDS_IN_SECOND);
  
  return () => clearTimeout(timeoutId);
}

Suggested change

	// Set timeout to refresh before expiry
	const timeoutId = setTimeout(() => {
	void fetchNode();
	}, refreshInSeconds * MILLISECONDS_IN_SECOND);
	return () => {
	clearTimeout(timeoutId);
	};
	// Only set timeout if there's meaningful time left
	if (refreshInSeconds > 0) {
	const timeoutId = setTimeout(() => {
	void fetchNode();
	}, refreshInSeconds * MILLISECONDS_IN_SECOND);
	return () => {
	clearTimeout(timeoutId);
	};
	}

[Copilot]

[Important] The iframe lacks security attributes recommended for embedding external content. Add sandbox and other security attributes:

<iframe
  css={styles.editorIframe}
  src={node.EditorURLs?.collabora.Url}
  title={t('fileFullscreenModal.editor.iframeTitle')}
  sandbox="allow-scripts allow-same-origin allow-forms allow-popups allow-modals"
  referrerPolicy="no-referrer"
/>

Note: Adjust the sandbox permissions based on actual Collabora requirements - use the most restrictive set that still allows the editor to function.

[Copilot]

[Important] The toggle buttons lack proper accessibility attributes. They should include role="button", type="button", and use aria-pressed instead of relying only on CSS classes for state indication.

<button
  type="button"
  role="button"
  title="Viewing"
  aria-label="Viewing"
  aria-pressed={!isInEditMode}
  className={!isInEditMode ? 'active' : ''}
  onClick={() => onEditModeChange(false)}
>
  <ShowIcon width={16} height={16} />
  Viewing
</button>

Apply the same pattern to the "Editing" button with aria-pressed={isInEditMode}.

[Copilot]

[Bug] Using Number() on ExpiresAt can produce NaN if the value is not a valid number. This would make refreshInSeconds also NaN, leading to unexpected behavior. Add validation:

const expiresInSeconds = Number(node.EditorURLs.collabora.ExpiresAt);

if (isNaN(expiresInSeconds) || expiresInSeconds <= 0) {
  return;
}

const refreshInSeconds = Math.max(0, expiresInSeconds - REFRESH_BUFFER_SECONDS);

if (refreshInSeconds > 0) {
  const timeoutId = setTimeout(() => {
    void fetchNode();
  }, refreshInSeconds * MILLISECONDS_IN_SECOND);

  return () => clearTimeout(timeoutId);
}

Suggested change

	const refreshInSeconds = expiresInSeconds - REFRESH_BUFFER_SECONDS;
	// Set timeout to refresh before expiry
	const timeoutId = setTimeout(() => {
	void fetchNode();
	}, refreshInSeconds * MILLISECONDS_IN_SECOND);
	return () => {
	clearTimeout(timeoutId);
	};
	if (isNaN(expiresInSeconds) || expiresInSeconds <= 0) {
	return;
	}
	const refreshInSeconds = Math.max(0, expiresInSeconds - REFRESH_BUFFER_SECONDS);
	// Set timeout to refresh before expiry
	if (refreshInSeconds > 0) {
	const timeoutId = setTimeout(() => {
	void fetchNode();
	}, refreshInSeconds * MILLISECONDS_IN_SECOND);
	return () => {
	clearTimeout(timeoutId);
	};
	}

[Copilot]

[Important] The download function fetches fresh node data but doesn't validate that PreSignedGET?.Url is present before falling back to fileUrl. If both are undefined or invalid URLs, the download may fail silently. Add validation:

const handleFileDownload = async () => {
  if (!fileUrl) return;
  
  try {
    const node = await cellsRepository.getNode({uuid: id, flags: []});
    const downloadUrl = node.PreSignedGET?.Url || fileUrl;
    
    if (!downloadUrl) {
      console.error('No valid download URL available');
      return;
    }
    
    await forcedDownloadFile({url: downloadUrl, name: fileNameWithExtension});
  } catch (error) {
    console.error('Failed to download file:', error);
  }
};

Suggested change

	if (fileUrl) {
	const node = await cellsRepository.getNode({uuid: id, flags: []});
	await forcedDownloadFile({url: node.PreSignedGET?.Url || fileUrl, name: fileNameWithExtension});
	if (!fileUrl) {
	// No fileUrl provided, cannot proceed
	console.error('No fileUrl provided for download');
	return;
	}
	try {
	const node = await cellsRepository.getNode({uuid: id, flags: []});
	const downloadUrl = node.PreSignedGET?.Url || fileUrl;
	if (!downloadUrl || typeof downloadUrl !== 'string' || downloadUrl.trim() === '') {
	console.error('No valid download URL available');
	return;
	}
	await forcedDownloadFile({url: downloadUrl, name: fileNameWithExtension});
	} catch (error) {
	console.error('Failed to download file:', error);

[Copilot]

[nitpick] [Suggestion] The isFileEditable function and EDITABLE_FILE_EXTENSIONS constant lack JSDoc documentation explaining what makes a file editable and the business logic behind the supported extensions:

/**
 * File extensions that support collaborative editing via Collabora integration.
 * Includes modern Office formats (docx, xlsx, pptx) and OpenDocument formats (odf).
 */
const EDITABLE_FILE_EXTENSIONS = ['odf', 'docx', 'xlsx', 'pptx'];

/**
 * Checks if a file extension is supported for collaborative editing.
 * 
 * @param fileExtension - The file extension to check (without the leading dot)
 * @returns true if the file can be edited with Collabora, false otherwise
 * 
 * @example
 * isFileEditable('docx') // returns true
 * isFileEditable('pdf')  // returns false
 */
export const isFileEditable = (fileExtension: string): boolean => {
  return EDITABLE_FILE_EXTENSIONS.includes(fileExtension.toLowerCase());
};

Suggested change

	const EDITABLE_FILE_EXTENSIONS = ['odf', 'docx', 'xlsx', 'pptx'];
	/**
	* List of file extensions that are supported for collaborative editing via Collabora integration.
	*
	* Business logic:
	* - Only modern Office formats (docx, xlsx, pptx) and OpenDocument format (odf) are supported.
	* - These formats can be edited collaboratively in the application.
	* - Other formats (e.g., pdf, txt) are not supported for editing.
	*/
	const EDITABLE_FILE_EXTENSIONS = ['odf', 'docx', 'xlsx', 'pptx'];
	/**
	* Checks if a file extension is supported for collaborative editing.
	*
	* @param fileExtension - The file extension to check (without the leading dot).
	* @returns {boolean} True if the file can be edited with Collabora, false otherwise.
	*
	* @example
	* isFileEditable('docx') // returns true
	* isFileEditable('pdf') // returns false
	*/

[Copilot]

[Important] The iframe src attribute uses an unsanitized URL from the API response (node.EditorURLs?.collabora.Url). While this URL comes from a trusted backend, it should still be validated to ensure it uses HTTPS protocol and doesn't contain javascript: or data: schemes.

Consider adding URL validation:

const validateEditorUrl = (url: string): boolean => {
  try {
    const parsed = new URL(url);
    return parsed.protocol === 'https:';
  } catch {
    return false;
  }
};

// Then in the render:
if (!node.EditorURLs?.collabora.Url || !validateEditorUrl(node.EditorURLs.collabora.Url)) {
  return <div>{t('fileFullscreenModal.editor.error')}</div>;
}

[Copilot]

[nitpick] [Suggestion] The variable name isEditableState is somewhat redundant. Since it's already a local state variable, consider renaming to isInEditMode to match the prop name used in FileHeader (isInEditMode), improving consistency:

const [isInEditMode, setIsInEditMode] = useState(isEditMode);
const isEditable = isFileEditable(fileExtension);

const onCloseModal = () => {
  setIsInEditMode(false);
  onClose();
};

useEffect(() => {
  setIsInEditMode(!!isEditMode);
}, [isEditMode]);

[github-actions]

🔗 Download Full Report Artifact

🧪 Playwright Test Summary

• ✅ Passed: 9
• ❌ Failed: 4
• ⏭ Skipped: 0
• 🔁 Flaky: 1
• 📊 Total: 14
• ⏱ Total Runtime: 971.2s (~ 16 min 11 sec)

Failed Tests:

❌ Team owner adds whole team to an all team chat (tags: TC-8631, crit-flow-web)

Location: specs/CriticalFlow/addMembersToChat-TC-8631.spec.ts:43
Duration: 53806ms

Errors:

Error: expect(locator).toBeVisible() failed

Locator: locator('[data-uie-name="item-message"][data-uie-send-status="2"]').filter({ hasText: 'Hello from Alden!' }).locator('[data-uie-name="message-reactions"]').locator('[data-uie-name="emoji-pill"][title="heart"]')
Expected: visible
Timeout: 10000ms
Error: element(s) not found

Call log:
  - Expect "toBeVisible" with timeout 10000ms
  - waiting for locator('[data-uie-name="item-message"][data-uie-send-status="2"]').filter({ hasText: 'Hello from Alden!' }).locator('[data-uie-name="message-reactions"]').locator('[data-uie-name="emoji-pill"][title="heart"]')


  185 |       await pages.conversationList().openConversation(conversationName);
  186 |       const ownerMessage = pages.conversation().getMessage({content: `Hello from ${owner.firstName}!`});
> 187 |       await expect(pages.conversation().getReactionOnMessage(ownerMessage, 'heart')).toBeVisible();
      |                                                                                      ^
  188 |       await expect(pages.conversation().getReactionOnMessage(ownerMessage, 'joy')).toBeVisible();
  189 |
  190 |       // Member1 verifies they can see thumbs up (+1) and joy (😂) reactions on their message from owner and member2
    at /home/runner/actions-runner/_work/wire-webapp/wire-webapp/test/e2e_tests/specs/CriticalFlow/addMembersToChat-TC-8631.spec.ts:187:86
    at /home/runner/actions-runner/_work/wire-webapp/wire-webapp/test/e2e_tests/specs/CriticalFlow/addMembersToChat-TC-8631.spec.ts:183:5

❌ Planning group call with sending various messages during call (tags: TC-8632, crit-flow-web)

Location: specs/CriticalFlow/groupCalls-TC-8632.spec.ts:37
Duration: 31640ms

Errors:

Error: expect(received).toBeFalsy()

Received: true

  122 |       await memberCalling.unmuteSelfInFullScreen();
  123 |       await memberPageManager.waitForTimeout(250);
> 124 |       expect(await memberCalling.isSelfUserMutedInFullScreen()).toBeFalsy();
      |                                                                 ^
  125 |     });
  126 |
  127 |     await test.step('Validation: Owner sees member is unmuted', async () => {
    at /home/runner/actions-runner/_work/wire-webapp/wire-webapp/test/e2e_tests/specs/CriticalFlow/groupCalls-TC-8632.spec.ts:124:65
    at /home/runner/actions-runner/_work/wire-webapp/wire-webapp/test/e2e_tests/specs/CriticalFlow/groupCalls-TC-8632.spec.ts:121:5

❌ Group Video call (tags: TC-8637, crit-flow-web)

Location: specs/CriticalFlow/groupVideoCall-TC-8637.spec.ts:39
Duration: 41510ms

Errors:

TimeoutError: locator.waitFor: Timeout 20000ms exceeded.
Call log:
  - waiting for locator('#add-participants [data-uie-name="search-list"] [aria-label="Open profile of Tianna Haag"]').locator('input[type="checkbox"]')


   at pageManager/webapp/pages/conversationDetails.page.ts:70

  68 |       await userLocator.click();
  69 |       // Wait for the user to be selected (checkbox should be checked)
> 70 |       await userLocator.locator('input[type="checkbox"]').waitFor({state: 'attached'});
     |                                                           ^
  71 |     }
  72 |
  73 |     await this.page.locator(`${selectById('add-participants')} ${selectByDataAttribute('do-create')}`).click();
    at ConversationDetailsPage.addUsersToConversation (/home/runner/actions-runner/_work/wire-webapp/wire-webapp/test/e2e_tests/pageManager/webapp/pages/conversationDetails.page.ts:70:59)
    at /home/runner/actions-runner/_work/wire-webapp/wire-webapp/test/e2e_tests/specs/CriticalFlow/groupVideoCall-TC-8637.spec.ts:127:7
    at /home/runner/actions-runner/_work/wire-webapp/wire-webapp/test/e2e_tests/specs/CriticalFlow/groupVideoCall-TC-8637.spec.ts:123:5

❌ New person joins team and setups up device (tags: TC-8635, crit-flow-web)

Location: specs/CriticalFlow/joinTeam-TC-8635.spec.ts:38
Duration: 67087ms

Errors:

TimeoutError: locator.waitFor: Timeout 20000ms exceeded.
Call log:
  - waiting for locator('#add-participants [data-uie-name="search-list"] [aria-label="Open profile of Lexi Reichel"]').locator('input[type="checkbox"]')


   at pageManager/webapp/pages/conversationDetails.page.ts:70

  68 |       await userLocator.click();
  69 |       // Wait for the user to be selected (checkbox should be checked)
> 70 |       await userLocator.locator('input[type="checkbox"]').waitFor({state: 'attached'});
     |                                                           ^
  71 |     }
  72 |
  73 |     await this.page.locator(`${selectById('add-participants')} ${selectByDataAttribute('do-create')}`).click();
    at ConversationDetailsPage.addUsersToConversation (/home/runner/actions-runner/_work/wire-webapp/wire-webapp/test/e2e_tests/pageManager/webapp/pages/conversationDetails.page.ts:70:59)
    at /home/runner/actions-runner/_work/wire-webapp/wire-webapp/test/e2e_tests/specs/CriticalFlow/joinTeam-TC-8635.spec.ts:125:7
    at /home/runner/actions-runner/_work/wire-webapp/wire-webapp/test/e2e_tests/specs/CriticalFlow/joinTeam-TC-8635.spec.ts:114:5

Flaky Tests:

⚠️ Account Management (tags: TC-8639, crit-flow-web)

Location: specs/CriticalFlow/accountManagement-TC-8639.spec.ts:37

Attempt 1
Result: ❌ Failed
Duration: 85039ms

Errors:

Error: Matching URL not found in the email body

   at backend/inbucketClient.e2e.ts:120

  118 |
  119 |     if (this.isValidURL(matchingUrl) === false) {
> 120 |       throw new Error('Matching URL not found in the email body');
      |             ^
  121 |     }
  122 |
  123 |     return matchingUrl;
    at InbucketClientE2E.getMatchingURLFromEmailBody (/home/runner/actions-runner/_work/wire-webapp/wire-webapp/test/e2e_tests/backend/inbucketClient.e2e.ts:120:13)
    at /home/runner/actions-runner/_work/wire-webapp/wire-webapp/test/e2e_tests/specs/CriticalFlow/accountManagement-TC-8639.spec.ts:133:30
    at /home/runner/actions-runner/_work/wire-webapp/wire-webapp/test/e2e_tests/specs/CriticalFlow/accountManagement-TC-8639.spec.ts:124:3

Attempt 2
Result: ✅ Passed
Duration: 30342ms

[Copilot]

Pull request overview

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

[Copilot]

[Suggestion] The error handling doesn't capture or log the error details, making debugging difficult. When the API call fails, the error is silently swallowed.

Recommendation: Add proper error logging:

const fetchNode = useCallback(async () => {
  try {
    setIsLoading(true);
    setIsError(false);
    const fetchedNode = await cellsRepository.getNode({uuid: id, flags: ['WithEditorURLs']});
    setNode(fetchedNode);
  } catch (err) {
    console.error('Failed to fetch editor URLs:', err);
    setIsError(true);
  } finally {
    setIsLoading(false);
  }
}, [id, cellsRepository]);

[Copilot]

[nitpick] [Suggestion] The border styling appears to be missing from the iframe. For better visual consistency and to clearly separate the embedded content from the parent UI, consider adding a border property:

export const editorIframe: CSSObject = {
  width: '100%',
  height: '100%',
  border: 'none', // or '1px solid #ccc' if a border is desired
};

Suggested change

	height: '100%',
	height: '100%',
	border: '1px solid #ccc', // Ensures visual separation of iframe content

[Copilot]

[Suggestion] The new FileEditor component lacks test coverage. Given that other components in the codebase have comprehensive tests (e.g., FileTypeUtil.test.ts, FileAssetOptions.test.tsx), this critical component handling iframe integration should also have tests.

Recommendation: Add tests covering:

• Loading state display
• Error state display
• Successful iframe rendering
• URL refresh logic and timer handling
• Handling of missing or invalid EditorURLs

[Copilot]

Pull request overview

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

[Copilot]

[Important] The test suite explicitly verifies that odt, ods, and odp (standard OpenDocument formats) return false from isFileEditable(), while odf returns true. This seems backwards - .odt is the standard OpenDocument Text format that would typically be edited with Collabora, while .odf is for formulas.

If Collabora is meant to edit standard office documents, the editable list should likely include:

• odt (OpenDocument Text) instead of or in addition to odf
• Potentially ods (Spreadsheets) and odp (Presentations)

Recommendation: Verify with the Collabora integration requirements which formats are actually supported, and update both the EDITABLE_FILE_EXTENSIONS constant and these tests accordingly.

[Copilot]

[nitpick] [Suggestion] The iframe styling should include border: 'none' to remove the default iframe border, which is inconsistent with modern UI design.

Recommendation:

export const editorIframe: CSSObject = {
  width: '100%',
  height: '100%',
  border: 'none',
};

Suggested change

	height: '100%',
	height: '100%',
	border: 'none',

[Copilot]

Pull request overview

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

[Copilot]

[Important] Potential for extremely large or negative timeout values. If ExpiresAt is very large or refreshInSeconds is negative, this could cause issues.

Add validation:

const expiresInSeconds = Number(node.EditorURLs.collabora.ExpiresAt);

// Validate the expiry time is reasonable
if (isNaN(expiresInSeconds) || expiresInSeconds <= 0 || expiresInSeconds > 86400) {
  // Invalid or unreasonable expiry (> 24 hours)
  return undefined;
}

const refreshInSeconds = expiresInSeconds - REFRESH_BUFFER_SECONDS;

// Don't set timeout if already expired or refresh time is negative
if (refreshInSeconds <= 0) {
  return undefined;
}

const timeoutId = setTimeout(() => {
  void fetchNode();
}, refreshInSeconds * MILLISECONDS_IN_SECOND);

This prevents setting invalid timeouts that could cause unexpected behavior.

[Copilot]

[Important] The download URL from node.PreSignedGET?.Url is not validated before use. If the API returns a malicious URL, this could lead to security issues.

Add URL validation:

import {sanitizeUrl} from 'Components/InputBar/InputBarEditor/RichTextEditor/utils/url';

const handleFileDownload = async () => {
  if (fileUrl) {
    const node = await cellsRepository.getNode({uuid: id});
    const downloadUrl = node.PreSignedGET?.Url || fileUrl;
    const sanitizedUrl = sanitizeUrl(downloadUrl);
    
    if (!sanitizedUrl) {
      // Handle invalid URL error
      return;
    }
    
    await forcedDownloadFile({url: sanitizedUrl, name: fileNameWithExtension});
  }
};

This ensures only valid HTTPS URLs are used for downloads.

[Copilot]

[nitpick] [Suggestion] The error state renders a plain <div> with text, which is inconsistent with how errors are handled elsewhere in FileFullscreenModal (e.g., NoPreviewAvailable uses FilePlaceholder).

For consistency, use a proper error UI component:

import {FilePlaceholder} from '../common/FilePlaceholder/FilePlaceholder';

if (isError || !node) {
  return (
    <FilePlaceholder
      title={t('fileFullscreenModal.editor.error')}
      description={t('fileFullscreenModal.editor.errorDescription')}
    />
  );
}

This provides a better user experience with properly styled error messaging.

[Copilot]

[Suggestion] The parameter name isEditable in the onEditModeChange callback is misleading. The parameter actually represents whether edit mode should be enabled, not whether the file is editable.

Rename for clarity:

onEditModeChange: (shouldEnableEditMode: boolean) => void;

This makes it clear that the parameter controls the edit mode state, not file editability.

Suggested change

	onEditModeChange: (isEditable: boolean) => void;
	onEditModeChange: (shouldEnableEditMode: boolean) => void;

[zskhan]

Moving these folder and file into enum would be better
This can be tackled in another PR.

[zskhan]

We can use TIME_IN_MILLIS.SECOND instead

[Copilot]

Pull request overview

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

[Copilot]

[nitpick] [Suggestion] The auto-refresh effect could cause memory leaks if fetchNode changes frequently. The dependency array includes fetchNode, which is memoized with useCallback, but if the component re-renders and fetchNode is recreated, a new timeout will be set without clearing the old one properly.

Consider removing fetchNode from the dependency array and using a ref pattern:

useEffect(() => {
  if (!node?.EditorURLs?.collabora.ExpiresAt) {
    return undefined;
  }

  const expiresInSeconds = Number(node.EditorURLs.collabora.ExpiresAt);
  const refreshInSeconds = Math.max(0, expiresInSeconds - REFRESH_BUFFER_SECONDS);

  if (refreshInSeconds <= 0) {
    return undefined;
  }

  const timeoutId = setTimeout(() => {
    void fetchNode();
  }, refreshInSeconds * MILLISECONDS_IN_SECOND);

  return () => {
    clearTimeout(timeoutId);
  };
}, [node]); // Remove fetchNode from dependencies

[Copilot]

[nitpick] [Suggestion] Error handling is incomplete - the err is caught but not logged. This makes debugging production issues difficult.

Add error logging:

} catch (err) {
  console.error('Failed to fetch node with editor URLs:', err);
  setIsError(true);
}

Suggested change

	} catch (err) {
	} catch (err) {
	console.error('Failed to fetch node with editor URLs:', err);

[codecov]

Codecov Report

❌ Patch coverage is 8.73786% with 94 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.71%. Comparing base (85d9151) to head (6dd1b40).
⚠️ Report is 1 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev   #19813      +/-   ##
==========================================
- Coverage   43.78%   43.71%   -0.08%     
==========================================
  Files        1296     1298       +2     
  Lines       32637    32707      +70     
  Branches     7244     7259      +15     
==========================================
+ Hits        14290    14297       +7     
- Misses      16631    16693      +62     
- Partials     1716     1717       +1     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sonarqubecloud]

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
4.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

Pull request overview

Copilot reviewed 58 out of 60 changed files in this pull request and generated no new comments.