Added option to prevent sending requests to twenty-icons #16723
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 14 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-front/src/modules/settings/workspace/components/SettingsSecurityOther.tsx">
<violation number="1" location="packages/twenty-front/src/modules/settings/workspace/components/SettingsSecurityOther.tsx:16">
P1: Styled component `StyledToggleRequests` is defined inside the component function. This causes the styled component to be recreated on every render, leading to performance issues. Move it outside the component, following the pattern used in `NameField.tsx` in the same directory.</violation>
</file>
<file name="packages/twenty-front/src/modules/object-metadata/utils/getAvatarUrl.ts">
<violation number="1" location="packages/twenty-front/src/modules/object-metadata/utils/getAvatarUrl.ts:26">
P0: Using `useRecoilState` inside a regular utility function violates React's Rules of Hooks. This will cause runtime errors because hooks can only be called from React components or custom hooks.
Consider either:
1. Converting this to a custom hook (rename to `useGetAvatarUrl`)
2. Passing `currentWorkspace` as a parameter to this function
3. Using Recoil's `snapshot_UNSTABLE` API for reading state outside React components
Suppressing the ESLint rule doesn't fix the underlying issue - it just hides the warning.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| import { useDebouncedCallback } from 'use-debounce'; | ||
|
|
||
| export const SettingsSecurityOther = () => { | ||
| const StyledToggleRequests = styled.div` |
There was a problem hiding this comment.
P1: Styled component StyledToggleRequests is defined inside the component function. This causes the styled component to be recreated on every render, leading to performance issues. Move it outside the component, following the pattern used in NameField.tsx in the same directory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/settings/workspace/components/SettingsSecurityOther.tsx, line 16:
<comment>Styled component `StyledToggleRequests` is defined inside the component function. This causes the styled component to be recreated on every render, leading to performance issues. Move it outside the component, following the pattern used in `NameField.tsx` in the same directory.</comment>
<file context>
@@ -0,0 +1,115 @@
+import { useDebouncedCallback } from 'use-debounce';
+
+export const SettingsSecurityOther = () => {
+ const StyledToggleRequests = styled.div`
+ display: flex;
+ flex-direction: column;
</file context>
✅ Addressed in 15642a3
|
|
||
| if (objectNameSingular === CoreObjectNameSingular.Company) { | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| const [currentWorkspace] = useRecoilState(currentWorkspaceState); |
There was a problem hiding this comment.
P0: Using useRecoilState inside a regular utility function violates React's Rules of Hooks. This will cause runtime errors because hooks can only be called from React components or custom hooks.
Consider either:
- Converting this to a custom hook (rename to
useGetAvatarUrl) - Passing
currentWorkspaceas a parameter to this function - Using Recoil's
snapshot_UNSTABLEAPI for reading state outside React components
Suppressing the ESLint rule doesn't fix the underlying issue - it just hides the warning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/object-metadata/utils/getAvatarUrl.ts, line 26:
<comment>Using `useRecoilState` inside a regular utility function violates React's Rules of Hooks. This will cause runtime errors because hooks can only be called from React components or custom hooks.
Consider either:
1. Converting this to a custom hook (rename to `useGetAvatarUrl`)
2. Passing `currentWorkspace` as a parameter to this function
3. Using Recoil's `snapshot_UNSTABLE` API for reading state outside React components
Suppressing the ESLint rule doesn't fix the underlying issue - it just hides the warning.</comment>
<file context>
@@ -20,7 +22,13 @@ export const getAvatarUrl = (
- if (objectNameSingular === CoreObjectNameSingular.Company) {
+ // eslint-disable-next-line react-hooks/rules-of-hooks
+ const [currentWorkspace] = useRecoilState(currentWorkspaceState);
+
+ if (
</file context>
✅ Addressed in 12ae5f7
There was a problem hiding this comment.
Pull request overview
This PR adds a configurable workspace setting to control whether external requests are made to the twenty-icons service for fetching company logos. The feature allows administrators to disable external icon requests for privacy or security reasons.
Key Changes:
- Added a new
allowRequestsboolean field to the workspace entity with a default value oftrue - Created a new UI toggle in the Security settings to control this feature
- Modified the
getAvatarUrlutility to conditionally fetch company logos based on the workspace setting
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
packages/twenty-server/src/engine/core-modules/workspace/workspace.entity.ts |
Added allowRequests boolean field to workspace entity |
packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts |
Added permission flag for the new allowRequests field |
packages/twenty-server/src/engine/core-modules/workspace/dtos/update-workspace-input.ts |
Added allowRequests as an optional field in the update DTO |
packages/twenty-server/src/database/typeorm/core/migrations/common/1766176556359-allowRequests.ts |
Database migration to add the allowRequests column with default value true |
packages/twenty-front/src/testing/mock-data/users.ts |
Updated mock workspace data with allowRequests: true |
packages/twenty-front/src/pages/settings/security/SettingsSecurity.tsx |
Refactored to extract "Other" security settings into a separate component |
packages/twenty-front/src/modules/users/graphql/fragments/userQueryFragment.ts |
Added allowRequests field to the user query fragment |
packages/twenty-front/src/modules/settings/workspace/components/SettingsSecurityOther.tsx |
New component with UI controls for trash retention and icon request settings |
packages/twenty-front/src/modules/object-metadata/utils/getAvatarUrl.ts |
Modified to check allowRequests flag before fetching company icons from twenty-icons |
packages/twenty-front/src/modules/object-metadata/hooks/__tests__/useColumnDefinitionsFromObjectMetadata.test.ts |
Updated test mock with allowRequests: true |
packages/twenty-front/src/modules/auth/states/currentWorkspaceState.ts |
Added allowRequests to the CurrentWorkspace type definition |
packages/twenty-front/src/modules/apollo/services/__tests__/apollo.factory.test.ts |
Updated test mock with allowRequests: true |
packages/twenty-front/src/generated/graphql.ts |
Generated GraphQL type definitions for the new field |
packages/twenty-front/src/generated-metadata/graphql.ts |
Generated GraphQL metadata type definitions for the new field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| const [currentWorkspace] = useRecoilState(currentWorkspaceState); |
There was a problem hiding this comment.
This is a critical React Hooks violation. The function getAvatarUrl is a utility function (not a React component or custom hook), but it's calling useRecoilState, which is a React Hook. React Hooks can only be called from React components or custom hooks (functions starting with "use").
This will cause runtime errors because getAvatarUrl is called from various places in the codebase that are not within a React component's render cycle. The function needs to be refactored to accept the currentWorkspace value as a parameter instead of calling the hook directly.
packages/twenty-front/src/modules/settings/workspace/components/SettingsSecurityOther.tsx
Outdated
Show resolved
Hide resolved
| ...currentWorkspace, | ||
| allowRequests: value, | ||
| }); | ||
| } catch (err: any) { |
There was a problem hiding this comment.
The error handler uses any type annotation for the err parameter. This should use a more specific type. Consider using unknown and then checking the type, or using a union type like Error | ApolloError for better type safety.
| } catch (err: any) { | |
| } catch (err: unknown) { |
| } | ||
|
|
||
| if (objectNameSingular === CoreObjectNameSingular.Company) { | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks |
There was a problem hiding this comment.
An eslint-disable comment is being used to suppress the React Hooks rules violation, but this doesn't fix the underlying issue. The code will still fail at runtime because React Hooks cannot be called from regular functions. This suppression should be removed, and the code should be properly refactored to pass the workspace state as a parameter instead of calling the hook.
packages/twenty-server/src/engine/core-modules/workspace/workspace.entity.ts
Outdated
Show resolved
Hide resolved
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:10973 This environment will automatically shut down when the PR is closed or after 5 hours. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| if (objectNameSingular === CoreObjectNameSingular.Company) { | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| const [currentWorkspace] = useRecoilState(currentWorkspaceState); | ||
|
|
||
| if ( | ||
| objectNameSingular === CoreObjectNameSingular.Company && | ||
| currentWorkspace?.allowExternalRequests === true | ||
| ) { | ||
| return getLogoUrlFromDomainName( | ||
| getCompanyDomainName(record as Company) ?? '', | ||
| ); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
I'd be more than happy if someone could tell me how to solve problem with improper React hook as I don't see passing value several functions as a proper solution |
BOHEUS
force-pushed
the
no-requests-to-twenty-icons
branch
from
a3753f8 to
d0a89c1
Compare
packages/twenty-front/src/modules/object-metadata/utils/getObjectRecordIdentifier.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
The way we would do it is through a CONFIG VARIABLE not via a database field (server-level not workspace level). I think I'll close the PR for now because it requires a different approach but feel free to re-open starting from scratch.
Also "allowExternalRequest" is too broad, we try to name things so that there's no doubt what this is about when you read them.
Thanks!!
There was a problem hiding this comment.
Okay, thanks for information, I'll work on it in nearest future then 👍
|
/twenty pr open |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
@FelixMalfait solution changed to client config available in Admin Panel, also covers cases with the non-chip avatars like in CommandMenuPageInfoLayout or ShowPageSummaryCard Screencast.From.2026-01-19.18-51-01.mp4 |
There was a problem hiding this comment.
2 issues found across 17 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-front/src/modules/client-config/hooks/useClientConfig.ts">
<violation number="1" location="packages/twenty-front/src/modules/client-config/hooks/useClientConfig.ts:191">
P2: Missing default fallback for optional allowRequestsToTwentyIcons; undefined response overrides default true and disables icon requests with older servers</violation>
</file>
<file name="packages/twenty-front/src/modules/object-metadata/utils/getAvatarUrl.ts">
<violation number="1" location="packages/twenty-front/src/modules/object-metadata/utils/getAvatarUrl.ts:18">
P2: Regression: new allowRequestsToTwentyIcons flag defaults to undefined, so existing calls now skip company logo URLs and return empty avatar for companies without image field</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| setIsCloudflareIntegrationEnabled( | ||
| clientConfig?.isCloudflareIntegrationEnabled, | ||
| ); | ||
| setAllowRequestsToTwentyIcons(clientConfig?.allowRequestsToTwentyIcons); |
There was a problem hiding this comment.
P2: Missing default fallback for optional allowRequestsToTwentyIcons; undefined response overrides default true and disables icon requests with older servers
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/client-config/hooks/useClientConfig.ts, line 191:
<comment>Missing default fallback for optional allowRequestsToTwentyIcons; undefined response overrides default true and disables icon requests with older servers</comment>
<file context>
@@ -184,6 +188,7 @@ export const useClientConfig = (): UseClientConfigResult => {
setCalendarBookingPageId(clientConfig?.calendarBookingPageId ?? null);
setIsImapSmtpCaldavEnabled(clientConfig?.isImapSmtpCaldavEnabled);
setIsEmailingDomainsEnabled(clientConfig?.isEmailingDomainsEnabled);
+ setAllowRequestsToTwentyIcons(clientConfig?.allowRequestsToTwentyIcons);
} catch (err) {
const error =
</file context>
| setAllowRequestsToTwentyIcons(clientConfig?.allowRequestsToTwentyIcons); | |
| setAllowRequestsToTwentyIcons( | |
| clientConfig?.allowRequestsToTwentyIcons ?? true, | |
| ); |
| @@ -15,12 +15,16 @@ export const getAvatarUrl = ( | |||
| objectNameSingular: string, | |||
| record: ObjectRecord, | |||
| imageIdentifierFieldMetadataItem: FieldMetadataItem | undefined, | |||
| allowRequestsToTwentyIcons?: boolean | undefined, | |||
There was a problem hiding this comment.
P2: Regression: new allowRequestsToTwentyIcons flag defaults to undefined, so existing calls now skip company logo URLs and return empty avatar for companies without image field
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/object-metadata/utils/getAvatarUrl.ts, line 18:
<comment>Regression: new allowRequestsToTwentyIcons flag defaults to undefined, so existing calls now skip company logo URLs and return empty avatar for companies without image field</comment>
<file context>
@@ -15,12 +15,16 @@ export const getAvatarUrl = (
objectNameSingular: string,
record: ObjectRecord,
imageIdentifierFieldMetadataItem: FieldMetadataItem | undefined,
+ allowRequestsToTwentyIcons?: boolean | undefined,
) => {
if (objectNameSingular === CoreObjectNameSingular.WorkspaceMember) {
</file context>
Greptile SummaryThis PR adds a new configuration option What Changed:
Critical Issues Found:
Impact: Confidence Score: 1/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant ClientConfigAPI
participant Backend
participant TwentyIcons as twenty-icons.com
User->>Frontend: Load Application
Frontend->>ClientConfigAPI: GET /client-config
ClientConfigAPI->>Backend: Read ALLOW_REQUESTS_TO_TWENTY_ICONS
Backend-->>ClientConfigAPI: Config {allowRequestsToTwentyIcons: true/false}
ClientConfigAPI-->>Frontend: Client Config
Frontend->>Frontend: Store in allowRequestsToTwentyIconsState
alt Configuration Check Implemented (2 locations)
User->>Frontend: View Company in SummaryCard/CommandMenu
Frontend->>Frontend: Read allowRequestsToTwentyIconsState
alt allowRequestsToTwentyIcons === true
Frontend->>TwentyIcons: GET /company-domain.com
TwentyIcons-->>Frontend: Company Logo
else allowRequestsToTwentyIcons === false
Frontend->>Frontend: Use fallback
end
end
alt Configuration Check Missing (6+ locations)
User->>Frontend: View Company in EventRow/CommandMenu utils/Hooks
Frontend->>TwentyIcons: GET /company-domain.com (UNCHECKED!)
Note over Frontend,TwentyIcons: Privacy control bypassed
TwentyIcons-->>Frontend: Company Logo
end
alt Server-side Search (CRITICAL ISSUE)
User->>Backend: Search Companies
Backend->>TwentyIcons: GET /company-domain.com (UNCHECKED!)
Note over Backend,TwentyIcons: Configuration completely ignored
TwentyIcons-->>Backend: Company Logo
Backend-->>Frontend: Search Results with logos
end
|
There was a problem hiding this comment.
Additional Comments (6)
-
packages/twenty-front/src/modules/activities/timeline-activities/components/EventRow.tsx, line 121-124 (link)logic: Missing
allowRequestsToTwentyIconsparameterThe call to
getObjectRecordIdentifierdoesn't pass theallowRequestsToTwentyIconsparameter. This means timeline activities will always make requests to twenty-icons for Company records, regardless of the configuration setting. -
packages/twenty-front/src/modules/command-menu/utils/getRecordContextText.ts, line 11 (link)logic: Missing
allowRequestsToTwentyIconsparameter - configuration not respected in command menu context text generation -
packages/twenty-front/src/modules/object-record/record-store/states/selectors/recordStoreIdentifiersSelector.ts, line 30-33 (link)logic: Missing
allowRequestsToTwentyIconsparameter - selector doesn't respect configuration when generating identifiers for multiple records -
packages/twenty-front/src/modules/object-metadata/hooks/useMapToObjectRecordIdentifier.ts, line 15 (link)logic: Missing
allowRequestsToTwentyIconsparameter - hook doesn't respect configuration -
packages/twenty-front/src/modules/object-metadata/hooks/useGetObjectRecordIdentifierByNameSingular.ts, line 21-24 (link)logic: Missing
allowRequestsToTwentyIconsparameter - hook doesn't respect configuration when getting record identifiers -
packages/twenty-server/src/engine/core-modules/search/services/search.service.ts, line 390-392 (link)logic: Critical: Server-side search doesn't respect
ALLOW_REQUESTS_TO_TWENTY_ICONSconfigurationThe search service makes requests to twenty-icons for all company records without checking the configuration. This bypasses the privacy control that this PR is supposed to add.
The
ALLOW_REQUESTS_TO_TWENTY_ICONSconfig value needs to be injected into this service and checked before callinggetLogoUrlFromDomainName.
17 files reviewed, 6 comments
2 participants
Solution for #15891
Screencast.From.2025-12-19.23-11-52.mp4