Migrate url tooling to twenty-shared #10440

charlesBochet · 2025-02-24T13:22:23Z

Migrate and unify URL tooling in twenty-shared.

We now have:

isValidHostname which follows our own business rules
a zod schema that can be re-used in different context and leverages is isValidHostname
isValidUrl on top of the zod schema
a getAbsoluteURl and getHostname on top of the zod schema

I have added a LOT of tests to cover all the cases I've found

Also fixes: #10147

greptile-apps

PR Summary

This PR migrates URL-related utilities from the frontend and server packages to the twenty-shared package, centralizing URL validation and manipulation functions for better code organization and reuse.

Modified regex pattern in packages/twenty-shared/src/utils/url/isDomain.ts changes domain validation behavior, potentially affecting handling of internationalized domains and multi-level TLDs
Duplicate test case for 'http://localhost:3000' in packages/twenty-shared/src/utils/url/__tests__/isValidUrl.test.ts needs to be removed
Test suite in isValidUrl.test.ts lacks coverage for edge cases like IDN domains and URLs with authentication credentials
Import paths across multiple components have been updated to use the shared package, ensuring consistent URL validation across the application

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

greptile-apps · 2025-02-24T13:28:52Z

packages/twenty-server/src/engine/utils/is-domain.ts

@@ -1,5 +1,5 @@
 export const isDomain = (url: string | undefined | null) =>
  !!url &&
-  /^((?!-))(xn--)?[a-z0-9][a-z0-9-_]{0,61}[a-z0-9]{0,1}\.(xn--)?([a-z0-9-]{1,61}|[a-z0-9-]{1,30}\.[a-z]{2,})$/.test(
+  /^((?!-))(xn--)?[a-z0-9][a-z0-9-_]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})?$/.test(


logic: The new regex pattern may incorrectly validate some invalid domains by making the TLD optional. Consider keeping TLD validation mandatory by removing the ? after (\.[a-z]{2,})

Suggested change

/^((?!-))(xn--)?[a-z0-9][a-z0-9-_]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})?$/.test(

/^((?!-))(xn--)?[a-z0-9][a-z0-9-_]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})$/.test(

greptile-apps · 2025-02-24T13:28:54Z

packages/twenty-front/tsconfig.build.json

@@ -18,5 +18,5 @@
    "src/**/*.d.ts",
    "src/**/*.ts",
    "src/**/*.tsx"
-  ]
+, "../twenty-shared/src/utils/url/absoluteUrlSchema.ts", "../twenty-shared/src/utils/url/is-domain.ts"  ]


logic: The file path 'is-domain.ts' doesn't match the actual file name 'isDomain.ts' in twenty-shared package. This will cause build errors.

Suggested change

, "../twenty-shared/src/utils/url/absoluteUrlSchema.ts", "../twenty-shared/src/utils/url/is-domain.ts" ]

, "../twenty-shared/src/utils/url/absoluteUrlSchema.ts", "../twenty-shared/src/utils/url/isDomain.ts" ]

greptile-apps · 2025-02-24T13:28:55Z

packages/twenty-front/tsconfig.build.json

@@ -18,5 +18,5 @@
    "src/**/*.d.ts",
    "src/**/*.ts",
    "src/**/*.tsx"
-  ]
+, "../twenty-shared/src/utils/url/absoluteUrlSchema.ts", "../twenty-shared/src/utils/url/is-domain.ts"  ]


style: The formatting of this line is inconsistent with the rest of the file. The new entries should be on separate lines with proper indentation.

greptile-apps · 2025-02-24T13:29:46Z

packages/twenty-shared/src/utils/url/__tests__/absoluteUrlSchema.test.ts

@@ -29,5 +29,6 @@ describe('absoluteUrlSchema', () => {
  it('fails for invalid urls', () => {
    expect(absoluteUrlSchema.safeParse('?o').success).toBe(false);
    expect(absoluteUrlSchema.safeParse('\\').success).toBe(false);
+    expect(absoluteUrlSchema.safeParse('https://2')).toBe(false);


logic: The safeParse() method returns an object with a success property, but here we're comparing the entire result directly to false. This will always fail since an object can't equal false. Should be absoluteUrlSchema.safeParse('https://2').success

Suggested change

expect(absoluteUrlSchema.safeParse('https://2')).toBe(false);

expect(absoluteUrlSchema.safeParse('https://2').success).toBe(false);

greptile-apps · 2025-02-24T13:31:06Z

packages/twenty-shared/src/utils/url/getUrlHostname.ts

@@ -1,4 +1,4 @@
-import { getAbsoluteUrl } from '~/utils/url/getAbsoluteUrl';
+import { getAbsoluteUrl } from 'src/utils/url/getAbsoluteUrl';


style: Import path 'src/utils/url/getAbsoluteUrl' may need to be relative ('../getAbsoluteUrl') since both files are in the same directory

greptile-apps · 2025-02-24T13:32:30Z

packages/twenty-shared/src/utils/url/isDomain.ts

+
+export const isDomain = (url: string | undefined | null) =>
+  isDefined(url) &&
+  /^((?!-))(xn--)?[a-z0-9][a-z0-9-_]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})$/.test(


logic: The regex pattern has a limitation - it doesn't properly handle domains with multiple levels (e.g. 'sub.example.co.uk'). Consider updating to handle these cases.

greptile-apps · 2025-02-24T13:32:31Z

packages/twenty-shared/src/utils/url/isDomain.ts

+
+export const isDomain = (url: string | undefined | null) =>
+  isDefined(url) &&
+  /^((?!-))(xn--)?[a-z0-9][a-z0-9-_]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})$/.test(


logic: The regex allows underscores in domain names which is technically invalid according to RFC 1035. The '' should be removed from the character class [a-z0-9-].

Suggested change

/^((?!-))(xn--)?[a-z0-9][a-z0-9-_]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})$/.test(

/^((?!-))(xn--)?[a-z0-9][a-z0-9-]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})$/.test(

greptile-apps · 2025-02-24T13:32:32Z

packages/twenty-shared/src/utils/url/isDomain.ts

+
+export const isDomain = (url: string | undefined | null) =>
+  isDefined(url) &&
+  /^((?!-))(xn--)?[a-z0-9][a-z0-9-_]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})$/.test(


style: The pattern enforces lowercase only ([a-z0-9]). Convert url.toLowerCase() before testing to handle uppercase domains.

Suggested change

/^((?!-))(xn--)?[a-z0-9][a-z0-9-_]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})$/.test(

/^((?!-))(xn--)?[a-z0-9][a-z0-9-_]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})$/.test(url.toLowerCase(),

Weiko

LGTM

Weiko · 2025-02-24T16:02:32Z

packages/twenty-shared/src/utils/url/absoluteUrlSchema.ts

+    .replace('https://', '')
+    .replace('http://', '');
+
+  console.log('valueWithoutProtocol', valueWithoutProtocol);


console log

Weiko · 2025-02-24T16:02:43Z

packages/twenty-shared/src/utils/url/absoluteUrlSchema.ts

+    return z.NEVER;
+  }
+
+  console.log('absoluteUrl', absoluteUrl);


Weiko · 2025-02-24T16:02:47Z

packages/twenty-shared/src/utils/url/absoluteUrlSchema.ts

+      return absoluteUrl;
+    }
+
+    console.log('url', url);


Migrate url tooling to twenty-shared

1a60358

greptile-apps bot reviewed Feb 24, 2025

View reviewed changes

charlesBochet added 6 commits February 24, 2025 16:31

Fix

9eb64fa

Fix

daac134

Fix

d4083e1

Fix

c65b1f4

Fix

576a478

Fix

a331f20

Weiko approved these changes Feb 24, 2025

View reviewed changes

Fix

97c7b4f

charlesBochet merged commit 9046a9a into main Feb 24, 2025
49 checks passed

charlesBochet deleted the prevent-invalid-link branch February 24, 2025 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate url tooling to twenty-shared #10440

Migrate url tooling to twenty-shared #10440

charlesBochet commented Feb 24, 2025 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Feb 24, 2025

greptile-apps bot Feb 24, 2025

greptile-apps bot Feb 24, 2025

greptile-apps bot Feb 24, 2025

greptile-apps bot Feb 24, 2025

greptile-apps bot Feb 24, 2025

greptile-apps bot Feb 24, 2025

greptile-apps bot Feb 24, 2025

Weiko left a comment

Weiko Feb 24, 2025

Weiko Feb 24, 2025

Weiko Feb 24, 2025

	/^((?!-))(xn--)?[a-z0-9][a-z0-9-_]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})?$/.test(
	/^((?!-))(xn--)?[a-z0-9][a-z0-9-_]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})$/.test(

	, "../twenty-shared/src/utils/url/absoluteUrlSchema.ts", "../twenty-shared/src/utils/url/is-domain.ts" ]
	, "../twenty-shared/src/utils/url/absoluteUrlSchema.ts", "../twenty-shared/src/utils/url/isDomain.ts" ]

	expect(absoluteUrlSchema.safeParse('https://2')).toBe(false);
	expect(absoluteUrlSchema.safeParse('https://2').success).toBe(false);

		@@ -1,4 +1,4 @@
		import { getAbsoluteUrl } from '~/utils/url/getAbsoluteUrl';
		import { getAbsoluteUrl } from 'src/utils/url/getAbsoluteUrl';

	/^((?!-))(xn--)?[a-z0-9][a-z0-9-_]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})$/.test(
	/^((?!-))(xn--)?[a-z0-9][a-z0-9-]{0,61}[a-z0-9]{0,1}\.[a-z0-9-]{1,61}(\.[a-z]{2,})$/.test(

Migrate url tooling to twenty-shared #10440

Migrate url tooling to twenty-shared #10440

Conversation

charlesBochet commented Feb 24, 2025 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Feb 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 24, 2025

Choose a reason for hiding this comment

Weiko left a comment

Choose a reason for hiding this comment

Weiko Feb 24, 2025

Choose a reason for hiding this comment

Weiko Feb 24, 2025

Choose a reason for hiding this comment

Weiko Feb 24, 2025

Choose a reason for hiding this comment

charlesBochet commented Feb 24, 2025 •

edited

Loading