Fix broken image urls in Settings > Profile and Invite To Workspace Email #8942
Conversation
| @@ -20,7 +20,7 @@ type SendInviteLinkEmailProps = { | |||
| firstName: string; | |||
| lastName: string; | |||
| }; | |||
| serverUrl?: string; | |||
| serverUrl: string; | |||
There was a problem hiding this comment.
Overall, I think we should stop authorizing undefined in our typings unless this is needed. The caller should handle the case. This will reduce code complexity
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR consolidates image URL handling across packages and fixes broken image URLs in settings and email templates by centralizing the getImageAbsoluteURI utility.
- Removed twenty-front's getImageAbsoluteURI implementation in favor of twenty-ui's version, though twenty-emails retains its copy due to architectural constraints
- Added token-based authentication for workspace logos in
workspace-invitation.resolver.tsusing FileService - Added null/undefined checks before calling getImageAbsoluteURI in components like Avatar and Logo
- Fixed "File not found" errors when users connect via Google by adding proper null checks for workspace.logo
- Added FileModule to WorkspaceInvitationModule for handling authenticated logo URLs
💡 (5/5) You can turn off certain types of comments like style here!
16 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings | Greptile
| } | ||
|
|
||
| if (imageUrl?.startsWith('https:')) { | ||
| export const getImageAbsoluteURI = (imageUrl: string, serverUrl: string) => { |
There was a problem hiding this comment.
logic: Removing optional parameters could break existing code that passes undefined/null. Consider keeping parameters optional with default values.
| return imageUrl; | ||
| } | ||
|
|
||
| return serverUrl?.endsWith('/') | ||
| return serverUrl.endsWith('/') | ||
| ? `${serverUrl.substring(0, serverUrl.length - 1)}/files/${imageUrl}` | ||
| : `${serverUrl || ''}/files/${imageUrl}`; |
There was a problem hiding this comment.
logic: Using serverUrl || '' is redundant since serverUrl is now required. This fallback should be removed.
| getImageAbsoluteURI( | ||
| workspace.logo === null ? DEFAULT_WORKSPACE_LOGO : workspace.logo, | ||
| isDefined(workspace.logo) ? workspace.logo : DEFAULT_WORKSPACE_LOGO, | ||
| ) ?? '', |
There was a problem hiding this comment.
logic: This logic could still result in undefined being passed to getImageAbsoluteURI if workspace.logo is null. Consider using workspace.logo ?? DEFAULT_WORKSPACE_LOGO instead.
| @@ -15,6 +15,7 @@ export class FilePathGuard implements CanActivate { | |||
| async canActivate(context: ExecutionContext): Promise<boolean> { | |||
| const request = context.switchToHttp().getRequest(); | |||
| const query = request.query; | |||
| const urlParamsV2 = new URLSearchParams(request.originalUrl); | |||
There was a problem hiding this comment.
logic: urlParamsV2 is declared but never used
There was a problem hiding this comment.
logic: Removing this file removes null/undefined handling and proper URL path construction. The twenty-ui version may need these features added back to maintain full functionality.
There was a problem hiding this comment.
style: The URL construction using URL class was more robust than string concatenation. Consider keeping this approach for better URL handling.
| if (workspace.logo) { | ||
| const workspaceLogoToken = await this.fileService.encodeFileToken({ | ||
| workspaceId: workspace.id, | ||
| }); | ||
|
|
||
| workspaceLogoWithToken = `${workspace.logo}?token=${workspaceLogoToken}`; | ||
| } |
There was a problem hiding this comment.
style: token generation should be wrapped in try/catch to handle potential encoding errors gracefully
| return await this.workspaceInvitationService.sendInvitations( | ||
| sendInviteLinkInput.emails, | ||
| workspace, | ||
| { ...workspace, logo: workspaceLogoWithToken }, |
There was a problem hiding this comment.
logic: spreading workspace and overriding logo could cause issues if workspace is frozen/sealed object
| it('should return absolute url if the imageUrl is an absolute url', () => { | ||
| const imageUrl = 'https://XXX'; | ||
| const result = getImageAbsoluteURI(imageUrl); | ||
| expect(result).toBeNull(); | ||
| expect(result).toBe(imageUrl); | ||
| }); | ||
|
|
||
| it('should return absolute url if the imageUrl is an absolute url', () => { | ||
| const imageUrl = 'https://XXX'; | ||
| it('should return absolute url if the imageUrl is an absolute unsecure url', () => { | ||
| const imageUrl = 'http://XXX'; | ||
| const result = getImageAbsoluteURI(imageUrl); | ||
| expect(result).toBe(imageUrl); | ||
| }); |
There was a problem hiding this comment.
style: test cases for https and http are redundant since they test the same logic branch in the implementation
| } | ||
|
|
||
| if (imageUrl?.startsWith('http')) { | ||
| export const getImageAbsoluteURI = (imageUrl: string) => { |
There was a problem hiding this comment.
logic: Removing null/undefined handling could break existing code that passes undefined. Consider keeping parameter optional or adding runtime validation.
PR Summary: 1. Added `Twenty Shared` Package to centralize utilitiies as mentioned in #8942 2. Optimization of `getImageAbsoluteURI.ts` to handle edge cases  --------- Co-authored-by: Antoine Moreaux <[email protected]> Co-authored-by: Charles Bochet <[email protected]>
Fixes #8601
We had 3 implementations of getImageAbsoluteURI: in twenty-front, in twenty-ui and in twenty-emails. I was able to remove the one in twenty-front but I could not remove it from twenty-emails as this is a standalone for now. The vision is to introduce shared utils in a twenty-shared package