-
Notifications
You must be signed in to change notification settings - Fork 298
feat: integrate Collabora using iframe [WPB-21650] #19813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 11 commits
4390446
73c1cf5
a25e6ad
1ae5132
a3e8bb2
6419238
78dc1a0
00e58da
2b40436
57bbac6
70fd1bd
8f654e6
921da51
a00795c
a57f9d1
6dd1b40
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,25 @@ | ||||||||||||||||||||
| /* | ||||||||||||||||||||
| * Wire | ||||||||||||||||||||
| * Copyright (C) 2025 Wire Swiss GmbH | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * 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 3 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, see http://www.gnu.org/licenses/. | ||||||||||||||||||||
| * | ||||||||||||||||||||
| */ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import {CSSObject} from '@emotion/react'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| export const editorIframe: CSSObject = { | ||||||||||||||||||||
| width: '100%', | ||||||||||||||||||||
| height: '100%', | ||||||||||||||||||||
|
||||||||||||||||||||
| height: '100%', | |
| height: '100%', | |
| border: '0', |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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
};| height: '100%', | |
| height: '100%', | |
| border: '1px solid #ccc', // Ensures visual separation of iframe content |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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',
};| height: '100%', | |
| height: '100%', | |
| border: 'none', |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,97 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Wire | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Copyright (C) 2025 Wire Swiss GmbH | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * 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 3 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, see http://www.gnu.org/licenses/. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {useCallback, useEffect, useState} from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {Node} from '@wireapp/api-client/lib/cells'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {container} from 'tsyringe'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {CellsRepository} from 'Repositories/cells/CellsRepository'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {t} from 'Util/LocalizerUtil'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import * as styles from './FileEditor.styles'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {FileLoader} from '../FileLoader/FileLoader'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const MILLISECONDS_IN_SECOND = 1000; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const REFRESH_BUFFER_SECONDS = 10; // Refresh 10 seconds before expiry for safety | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| interface FileEditorProps { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export const FileEditor = ({id}: FileEditorProps) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const cellsRepository = container.resolve(CellsRepository); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [node, setNode] = useState<Node | null>(null); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [isLoading, setIsLoading] = useState(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const [isError, setIsError] = useState(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const fetchNode = useCallback(async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setIsLoading(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setIsError(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const fetchedNode = await cellsRepository.getNode({uuid: id, flags: ['WithEditorURLs']}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setNode(fetchedNode); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (err) { | |
| } catch (err) { | |
| console.error('Failed to fetch node with editor URLs:', err); |
Check warning on line 53 in src/script/components/FileFullscreenModal/FileEditor/FileEditor.tsx
SonarQubeCloud / SonarCloud Code Analysis
Handle this exception or don't catch it at all.
See more on https://sonarcloud.io/project/issues?id=wireapp_wire-webapp&issues=AZrLA7xPcXMBJo7p8zdZ&open=AZrLA7xPcXMBJo7p8zdZ&pullRequest=19813
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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]);Check failure on line 60 in src/script/components/FileFullscreenModal/FileEditor/FileEditor.tsx
SonarQubeCloud / SonarCloud Code Analysis
Remove this use of the "void" operator.
See more on https://sonarcloud.io/project/issues?id=wireapp_wire-webapp&issues=AZrLA7xPcXMBJo7p8zda&open=AZrLA7xPcXMBJo7p8zda&pullRequest=19813
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Bug] The useEffect dependency array is incomplete. The fetchNode callback is included, but it depends on cellsRepository, which is resolved inside the component. Since cellsRepository comes from a DI container, it's stable, but for correctness and to avoid future issues, consider one of these approaches:
- Move
cellsRepositoryoutside the callback:
const cellsRepository = container.resolve(CellsRepository);
const fetchNode = useCallback(async () => {
try {
setIsLoading(true);
setIsError(false);
const fetchedNode = await cellsRepository.getNode({uuid: id, flags: ['WithEditorURLs']});
setNode(fetchedNode);
} catch (err) {
setIsError(true);
} finally {
setIsLoading(false);
}
}, [id, cellsRepository]);- Or simplify by removing
cellsRepositoryfrom the dependency list since it's stable from the container.
| }, [id, cellsRepository]); | |
| // Initial fetch | |
| useEffect(() => { | |
| void fetchNode(); | |
| }, [id, cellsRepository, fetchNode]); | |
| }, [id]); | |
| // Initial fetch | |
| useEffect(() => { | |
| void fetchNode(); | |
| }, [id, fetchNode]); |
Check failure on line 74 in src/script/components/FileFullscreenModal/FileEditor/FileEditor.tsx
SonarQubeCloud / SonarCloud Code Analysis
Remove this use of the "void" operator.
See more on https://sonarcloud.io/project/issues?id=wireapp_wire-webapp&issues=AZrLA7xPcXMBJo7p8zdb&open=AZrLA7xPcXMBJo7p8zdb&pullRequest=19813
Outdated
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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);
}| // 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
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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);
}| 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
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] [Suggestion] The error display lacks proper styling and structure. Consider using a proper error component with consistent styling:
if (isError || !node) {
return (
<div css={{
display: 'flex',
alignItems: 'center',
justifyContent: 'center',
height: '100%',
padding: '2rem',
textAlign: 'center'
}}>
{t('fileFullscreenModal.editor.error')}
</div>
);
}| return <div>{t('fileFullscreenModal.editor.error')}</div>; | |
| return ( | |
| <div css={styles.errorContainer}> | |
| {t('fileFullscreenModal.editor.error')} | |
| </div> | |
| ); |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Check warning on line 92 in src/script/components/FileFullscreenModal/FileEditor/FileEditor.tsx
SonarQubeCloud / SonarCloud Code Analysis
Unknown property 'css' found
See more on https://sonarcloud.io/project/issues?id=wireapp_wire-webapp&issues=AZrLA7xPcXMBJo7p8zdc&open=AZrLA7xPcXMBJo7p8zdc&pullRequest=19813
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Important] There's no validation that node.EditorURLs?.collabora.Url is actually a string before using it as the iframe src. Add validation:
if (isError || !node || !node.EditorURLs?.collabora?.Url) {
return <div>{t('fileFullscreenModal.editor.error')}</div>;
}
const editorUrl = node.EditorURLs.collabora.Url;
return (
<iframe
css={styles.editorIframe}
src={editorUrl}
title={t('fileFullscreenModal.editor.iframeTitle')}
/>
);| if (isError || !node) { | |
| return <div>{t('fileFullscreenModal.editor.error')}</div>; | |
| } | |
| return ( | |
| <iframe | |
| css={styles.editorIframe} | |
| src={node.EditorURLs?.collabora.Url} | |
| const editorUrl = node?.EditorURLs?.collabora?.Url; | |
| if ( | |
| isError || | |
| !node || | |
| typeof editorUrl !== 'string' || | |
| editorUrl.trim() === '' | |
| ) { | |
| return <div>{t('fileFullscreenModal.editor.error')}</div>; | |
| } | |
| return ( | |
| <iframe | |
| css={styles.editorIframe} | |
| src={editorUrl} |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving these
folderandfileinto enum would be betterThis can be tackled in another PR.