Add Twenty Shared & Fix profile image rendering #8841

mdrazak2001 · 2024-12-03T04:41:31Z

PR Summary:

Added Twenty Shared Package to centralize utilitiies as mentioned in Fix broken image urls in Settings > Profile and Invite To Workspace Email #8942
Optimization of getImageAbsoluteURI.ts to handle edge cases

greptile-apps

PR Summary

This PR modifies the image URL construction logic to fix profile picture rendering issues, specifically addressing the "File not found" error when displaying profile images.

Modified packages/twenty-front/src/utils/image/getImageAbsoluteURI.ts to use direct string concatenation instead of URL object manipulation
Removed URL encoding/normalization to prevent issues with special characters in image URLs
Simplified handling of URL path construction by removing leading slash checks

_{💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!}

_{1 file(s) reviewed, 1 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-12-03T04:42:02Z

packages/twenty-front/src/utils/image/getImageAbsoluteURI.ts

-
-  return serverFilesUrl.toString() as ImageAbsoluteURI<T>;
-};
+  return `${serverFilesUrl}/files/${imageUrl}` as ImageAbsoluteURI<T>;


logic: No handling for leading slashes - if imageUrl starts with '/', the result will have double slashes ('//files//'). Add back the previous slash handling logic.

github-actions · 2024-12-03T04:42:23Z

Welcome!

Hello there, congrats on your first PR! We're excited to have you contributing to this project.
By submitting your Pull Request, you acknowledge that you agree with the terms of our Contributor License Agreement.

Generated by 🚫 dangerJS against 179c0d5

FelixMalfait · 2024-12-03T09:20:01Z

cc @AMoreaux fyi

Thank you @mdrazak2001!
I think it might be a good opportunity to create a twenty-shared... It's a mess to have 3 times the same function 😅

Do you think you could take a shot at it today? Or should we take over on your PR?
Thanks a lot

AMoreaux · 2024-12-03T09:33:43Z

Hello 👋

Thanks for the contribution.

Before adding the fix can you add a test including the URL pattern that broke the function?

In this file:
https://github.com/twentyhq/twenty/blob/main/packages/twenty-front/src/utils/image/__tests__/getImageAbsoluteURI.test.ts

Moreover, since you're on this file. Could you add this test? It validate the fact we manage the risk of double slash.

  it('should return fully formed url if imageUrl is a relative url and start with a /', () => {
    const imageUrl = '/XXX';
    const result = getImageAbsoluteURI(imageUrl);
    expect(result).toBe('http://localhost:3000/files/XXX');
  });

The URL API allows you to validate these kinds of use cases. You don't need to manage the double // or the validation of the URI yourself.

If you need help or have a lack of time feel free to tell me ;)

mdrazak2001 · 2024-12-03T12:38:33Z

cc @AMoreaux fyi

Thank you @mdrazak2001! I think it might be a good opportunity to create a twenty-shared... It's a mess to have 3 times the same function 😅

Do you think you could take a shot at it today? Or should we take over on your PR? Thanks a lot

Great suggestion! I'll work on creating a twenty-shared utility to consolidate this function across the codebase.
As part of consolidating this utility into a shared implementation, would you like me to replace all occurrences of getImageAbsoluteURI across the codebase and remove the duplicate implementations in this PR itself?

FelixMalfait · 2024-12-03T12:53:19Z

@mdrazak2001 yes that'd be great. It might be a bit difficult to do a good configuration so let us know if you're stuck!

mdrazak2001 · 2024-12-03T12:54:38Z

Hello 👋

Thanks for the contribution.

Before adding the fix can you add a test including the URL pattern that broke the function?

In this file: https://github.com/twentyhq/twenty/blob/main/packages/twenty-front/src/utils/image/__tests__/getImageAbsoluteURI.test.ts

Moreover, since you're on this file. Could you add this test? It validate the fact we manage the risk of double slash.
  it('should return fully formed url if imageUrl is a relative url and start with a /', () => {
    const imageUrl = '/XXX';
    const result = getImageAbsoluteURI(imageUrl);
    expect(result).toBe('http://localhost:3000/files/XXX');
  });
The URL API allows you to validate these kinds of use cases. You don't need to manage the double // or the validation of the URI yourself.

If you need help or have a lack of time feel free to tell me ;)

@AMoreaux:
Thank you for the detailed review!

I will add a test case for the specific URL pattern (?) that initially caused the bug to ensure the issue is addressed.
I’ll also include the additional test case you suggested, to validate proper handling of relative Image/File URLs starting with a /.

Planned Solution:
Upon reviewing, I identified that the root cause was the improper manual manipulation of pathname, which led to the incorrect encoding of ? as %3F. Based on your suggestion to leverage the URL API, I plan to implement a solution using its base URL resolution capabilities. This approach will ensure:

Proper handling of leading slashes.
Correct resolution of query parameters.
Prevention of unnecessary encoding issues.

I’ll make these updates shortly. Please let me know if there’s anything else you'd like me to address!

…ture issue (twentyhq#8601).

mdrazak2001 · 2024-12-05T12:09:40Z

Hi @FelixMalfait @AMoreaux
I’ve committed the initial setup and created the centralized twenty-shared library as suggested.
Please review it at your convenience and let me know if you have any concerns or feedback.
Thanks!

AMoreaux

Lots of works, thanks 🙏

Few comments. TLDR:

Use the barrel file
Remove all reference to react or tsx in twenty-shared
If you want you can remove the type in getImageAbsoluteURI.ts

I will test it in the browser asap.

AMoreaux · 2024-12-06T09:14:20Z

packages/twenty-shared/tsconfig.spec.json

+    "include": [
+      "**/__mocks__/**/*",
+      "jest.config.ts",
+      "setupTests.ts",
+      "src/**/*.d.ts",
+      "src/**/*.spec.ts",
+      "src/**/*.spec.tsx",
+      "src/**/*.test.ts",
+      "src/**/*.test.tsx",
+      "tsup.config.ts",
+      "tsup.ui.index.tsx",
+      "vite.config.ts"
+    ]


Suggested change

"include": [

"**/__mocks__/**/*",

"jest.config.ts",

"setupTests.ts",

"src/**/*.d.ts",

"src/**/*.spec.ts",

"src/**/*.spec.tsx",

"src/**/*.test.ts",

"src/**/*.test.tsx",

"tsup.config.ts",

"tsup.ui.index.tsx",

"vite.config.ts"

]

"include": [

"jest.config.ts",

"setupTests.ts",

"src/**/*.spec.ts",

"src/**/*.test.ts",

"vite.config.ts"

]

It seems some includes are not unused

AMoreaux · 2024-12-06T09:15:44Z

packages/twenty-shared/tsconfig.lib.json

+    "**/*.spec.jsx",
+    "**/*.test.jsx"
+  ],
+  "include": ["src/**/*.js", "src/**/*.jsx", "src/**/*.ts", "src/**/*.tsx"]


Suggested change

"include": ["src/**/*.js", "src/**/*.jsx", "src/**/*.ts", "src/**/*.tsx"]

"include": ["src/**/*.js", "src/**/*.jsx", "src/**/*.ts"]

This package shouldn't include tsx components since we have twenty-ui for that.

AMoreaux · 2024-12-06T09:22:34Z

packages/twenty-shared/.eslintrc.cjs

+  ignorePatterns: ['!**/*'],
+  overrides: [
+    {
+      files: ['*.ts', '*.tsx'],


Suggested change

files: ['*.ts', '*.tsx'],

files: ['*.ts'],

We will not have tsx in this package;

AMoreaux · 2024-12-06T09:23:13Z

packages/twenty-shared/.eslintrc.cjs

@@ -0,0 +1,15 @@
+module.exports = {
+  extends: ['../../.eslintrc.cjs', '../../.eslintrc.react.cjs'],


Suggested change

extends: ['../../.eslintrc.cjs', '../../.eslintrc.react.cjs'],

extends: ['../../.eslintrc.cjs'],

We will not use react in this package;

AMoreaux · 2024-12-06T09:25:04Z

packages/twenty-shared/jest.config.ts

+    global: {
+      statements: 60,
+      lines: 60,
+      functions: 50,
+    },


Suggested change

global: {

statements: 60,

lines: 60,

functions: 50,

},

global: {

statements: 95,

lines: 95,

functions: 95,

},

Since this package will be shared and should contain only easy-to-test functions we can have a high level of coverage.

AMoreaux · 2024-12-06T11:12:54Z

packages/twenty-shared/src/utils/image/getImageAbsoluteURI.ts

+type ImageAbsoluteURI<T extends string | null | undefined> = T extends string
+  ? string
+  : null;


We talked internally, and we would like to remove this type. It will impact the function's return type.
I planned to do it myself but if you want to do it feel free :)

AMoreaux · 2024-12-06T11:18:25Z

tsconfig.base.json

@@ -17,7 +17,8 @@
    "baseUrl": ".",
    "paths": {
      "twenty-emails": ["packages/twenty-emails/src/index.ts"],
-      "twenty-ui": ["packages/twenty-ui/src/index.ts"]
+      "twenty-ui": ["packages/twenty-ui/src/index.ts"],
+      "twenty-shared": ["packages/twenty-shared/dist"]


Suggested change

"twenty-shared": ["packages/twenty-shared/dist"]

"twenty-shared": ["packages/twenty-shared/src/index.ts"]

Please target the barrel file here packages/twenty-shared/src/index.ts instead of targeting dist

AMoreaux · 2024-12-06T11:18:33Z

packages/twenty-ui/tsconfig.json

@@ -10,7 +10,8 @@
    "types": ["node"],
    "outDir": "../../.cache/tsc",
    "paths": {
-      "@ui/*": ["packages/twenty-ui/src/*"]
+      "@ui/*": ["packages/twenty-ui/src/*"],
+      "twenty-shared": ["packages/twenty-shared/dist"]


Suggested change

"twenty-shared": ["packages/twenty-shared/dist"]

"twenty-shared": ["packages/twenty-shared/src/*"]

Please target the barrel file here packages/twenty-shared/src/index.ts instead of targeting dist

AMoreaux · 2024-12-06T11:22:03Z

packages/twenty-emails/tsconfig.json

-    "baseUrl": "."
+    "baseUrl": ".",
+    "paths": {
+      "twenty-shared": ["packages/twenty-shared/dist"]


Suggested change

"twenty-shared": ["packages/twenty-shared/dist"]

"twenty-shared": ["packages/twenty-shared/src/*"]

AMoreaux · 2024-12-06T11:23:09Z

packages/twenty-front/tsconfig.json

@@ -24,7 +24,8 @@
      "@/*": ["packages/twenty-front/src/modules/*"],
      "~/*": ["packages/twenty-front/src/*"],
      "twenty-ui": ["packages/twenty-ui/src/index.ts"],
-      "@ui/*": ["packages/twenty-ui/src/*"]
+      "@ui/*": ["packages/twenty-ui/src/*"],
+      "twenty-shared": ["packages/twenty-shared/src"]


Suggested change

"twenty-shared": ["packages/twenty-shared/src"]

"twenty-shared": ["packages/twenty-shared/src/index.ts"]

…profile-image-display # Conflicts: # packages/twenty-emails/src/emails/send-invite-link.email.tsx # packages/twenty-emails/src/utils/getImageAbsoluteURI.ts # packages/twenty-front/src/modules/auth/components/Logo.tsx # packages/twenty-front/src/modules/navigation/components/AppNavigationDrawer.tsx # packages/twenty-front/src/modules/object-metadata/utils/getAvatarUrl.ts # packages/twenty-front/src/modules/ui/input/components/ImageInput.tsx # packages/twenty-front/src/modules/ui/navigation/navigation-drawer/components/MultiWorkspaceDropdownButton.tsx # packages/twenty-front/src/modules/ui/utilities/page-favicon/components/PageFavicon.tsx # packages/twenty-front/src/pages/settings/admin-panel/SettingsAdminFeatureFlags.tsx # packages/twenty-front/src/pages/settings/serverless-functions/__stories__/SettingsServerlessFunctionDetail.stories.tsx # packages/twenty-ui/src/display/avatar/components/Avatar.tsx # packages/twenty-ui/src/utilities/image/__tests__/getImageAbsoluteURI.test.ts # packages/twenty-ui/src/utilities/image/getImageAbsoluteURI.ts

Refactor the code to streamline the usage of `getImageAbsoluteURI` by storing its result in a variable before rendering. Cleanup unnecessary imports and minor code improvements were applied across several files. This should enhance code readability and maintainability.

Refactor URI handling in `getImageAbsoluteURI` by separating imageUrl and serverUrl checks for better clarity. Update favicon link in HTML and ensure consistency by encompassing logo checking logic inside the JSX return statement. Additional import of `useGetPublicWorkspaceDataBySubdomain` in `WorkspaceProviderEffect` to manage loading state.

AMoreaux

Perfect 💯

Thanks a lot for your contribution. I am waiting for the approval of @FelixMalfait for the next steps.

charlesBochet · 2024-12-10T08:54:31Z

@mdrazak2001 thank you! I'll merge this one today. I have unfortunately merged another fix in the meantime but your PR is bringing more, so I'll rebase it on main and merge it :)

greptile-apps bot reviewed Dec 3, 2024

View reviewed changes

AMoreaux self-assigned this Dec 3, 2024

AMoreaux added -PR: awaiting author scope: front Issues that are affecting the frontend side only labels Dec 3, 2024

mdrazak2001 added 2 commits December 4, 2024 23:29

Fix profile image rendering

ffb3033

Add Twenty Shared package to centralize utilities and fix profile pic…

5be9a09

…ture issue (twentyhq#8601).

mdrazak2001 force-pushed the fix/profile-image-display branch from 87a2031 to 5be9a09 Compare December 4, 2024 18:00

mdrazak2001 changed the title ~~Fix profile image rendering~~ Add Twenty Shared & Fix profile image rendering Dec 5, 2024

mdrazak2001 added 2 commits December 5, 2024 19:08

Merge branch 'main' into fix/profile-image-display

cf24eb6

Use Shared Utility in PageFavicon

73bd7da

AMoreaux requested changes Dec 6, 2024

View reviewed changes

Address review comments

55d680d

mdrazak2001 requested a review from AMoreaux December 7, 2024 05:50

AMoreaux added 4 commits December 9, 2024 09:48

fix(): workspace provider effect

179c0d5

AMoreaux approved these changes Dec 9, 2024

View reviewed changes

FelixMalfait assigned charlesBochet and unassigned AMoreaux Dec 9, 2024

FelixMalfait requested a review from charlesBochet December 9, 2024 11:08

charlesBochet removed the -PR: awaiting author label Dec 10, 2024

charlesBochet added the -PR: awaiting review label Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Twenty Shared & Fix profile image rendering #8841

Add Twenty Shared & Fix profile image rendering #8841

mdrazak2001 commented Dec 3, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Dec 3, 2024

github-actions bot commented Dec 3, 2024 •

edited

Loading

FelixMalfait commented Dec 3, 2024

AMoreaux commented Dec 3, 2024 •

edited

Loading

mdrazak2001 commented Dec 3, 2024

FelixMalfait commented Dec 3, 2024

mdrazak2001 commented Dec 3, 2024 •

edited

Loading

mdrazak2001 commented Dec 5, 2024

AMoreaux left a comment

AMoreaux Dec 6, 2024

AMoreaux Dec 6, 2024

AMoreaux Dec 6, 2024

AMoreaux Dec 6, 2024

AMoreaux Dec 6, 2024

AMoreaux Dec 6, 2024

AMoreaux Dec 6, 2024

AMoreaux Dec 6, 2024

AMoreaux Dec 6, 2024

AMoreaux Dec 6, 2024

AMoreaux left a comment

charlesBochet commented Dec 10, 2024

	"include": ["src/*/.js", "src/*/.jsx", "src/*/.ts", "src/*/.tsx"]
	"include": ["src/*/.js", "src/*/.jsx", "src/*/.ts"]

		@@ -0,0 +1,15 @@
		module.exports = {
		extends: ['../../.eslintrc.cjs', '../../.eslintrc.react.cjs'],

	extends: ['../../.eslintrc.cjs', '../../.eslintrc.react.cjs'],
	extends: ['../../.eslintrc.cjs'],

	"twenty-shared": ["packages/twenty-shared/dist"]
	"twenty-shared": ["packages/twenty-shared/src/index.ts"]

Add Twenty Shared & Fix profile image rendering #8841

Are you sure you want to change the base?

Add Twenty Shared & Fix profile image rendering #8841

Conversation

mdrazak2001 commented Dec 3, 2024 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Dec 3, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 3, 2024 • edited Loading

Welcome!

FelixMalfait commented Dec 3, 2024

AMoreaux commented Dec 3, 2024 • edited Loading

mdrazak2001 commented Dec 3, 2024

FelixMalfait commented Dec 3, 2024

mdrazak2001 commented Dec 3, 2024 • edited Loading

mdrazak2001 commented Dec 5, 2024

AMoreaux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AMoreaux left a comment

Choose a reason for hiding this comment

charlesBochet commented Dec 10, 2024

mdrazak2001 commented Dec 3, 2024 •

edited

Loading

github-actions bot commented Dec 3, 2024 •

edited

Loading

AMoreaux commented Dec 3, 2024 •

edited

Loading

mdrazak2001 commented Dec 3, 2024 •

edited

Loading