[Apps] Content + permission tabs for marketplace and installed apps#17888
[Apps] Content + permission tabs for marketplace and installed apps#17888
Conversation
...-front/src/pages/settings/applications/tabs/SettingsAvailableApplicationDetailContentTab.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
4 issues found across 26 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/pages/settings/applications/tabs/SettingsAvailableApplicationDetailContentTab.tsx">
<violation number="1" location="packages/twenty-front/src/pages/settings/applications/tabs/SettingsAvailableApplicationDetailContentTab.tsx:109">
P1: Avoid throwing during render when object metadata is still loading; the state defaults to an empty array so this can crash the page before metadata arrives. Handle the missing metadata gracefully (e.g., return an empty list or skip the row until metadata is loaded).</violation>
</file>
<file name="packages/twenty-front/src/pages/settings/applications/tabs/SettingsApplicationPermissionsTab.tsx">
<violation number="1" location="packages/twenty-front/src/pages/settings/applications/tabs/SettingsApplicationPermissionsTab.tsx:139">
P2: `uuidv4()` inside `useMemo` generates new IDs on every recomputation, making the memoized result referentially unstable in its nested properties. Consider generating stable IDs outside the memo (e.g., using a deterministic hash of the permission's universal identifiers) or caching them with `useRef` to avoid downstream re-render cascades.</violation>
</file>
<file name="packages/twenty-server/src/engine/core-modules/application/services/mocked-marketplace-app.constant.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/application/services/mocked-marketplace-app.constant.ts:22">
P2: Several different field universal identifiers are set to the same UUID, which will collide and make field metadata/permissions ambiguous. Each field should have a unique universalIdentifier.</violation>
<violation number="2" location="packages/twenty-server/src/engine/core-modules/application/services/mocked-marketplace-app.constant.ts:193">
P2: fieldPermissions should reference the field’s universalIdentifier, not its name string. This entry won’t match the jobTitle field and will make permissions ineffective.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| : undefined; | ||
|
|
||
| if (!isDefined(objectMetadataItem)) { | ||
| throw new Error( |
There was a problem hiding this comment.
P1: Avoid throwing during render when object metadata is still loading; the state defaults to an empty array so this can crash the page before metadata arrives. Handle the missing metadata gracefully (e.g., return an empty list or skip the row until metadata is loaded).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/pages/settings/applications/tabs/SettingsAvailableApplicationDetailContentTab.tsx, line 109:
<comment>Avoid throwing during render when object metadata is still loading; the state defaults to an empty array so this can crash the page before metadata arrives. Handle the missing metadata gracefully (e.g., return an empty list or skip the row until metadata is loaded).</comment>
<file context>
@@ -0,0 +1,148 @@
+ : undefined;
+
+ if (!isDefined(objectMetadataItem)) {
+ throw new Error(
+ `Could not resolve object for universalIdentifier: ${group.objectUniversalIdentifier}`,
+ );
</file context>
| })), | ||
| fieldPermissions: defaultRole.fieldPermissions.map((permission) => ({ | ||
| __typename: 'FieldPermission' as const, | ||
| id: uuidv4(), |
There was a problem hiding this comment.
P2: uuidv4() inside useMemo generates new IDs on every recomputation, making the memoized result referentially unstable in its nested properties. Consider generating stable IDs outside the memo (e.g., using a deterministic hash of the permission's universal identifiers) or caching them with useRef to avoid downstream re-render cascades.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/pages/settings/applications/tabs/SettingsApplicationPermissionsTab.tsx, line 139:
<comment>`uuidv4()` inside `useMemo` generates new IDs on every recomputation, making the memoized result referentially unstable in its nested properties. Consider generating stable IDs outside the memo (e.g., using a deterministic hash of the permission's universal identifiers) or caching them with `useRef` to avoid downstream re-render cascades.</comment>
<file context>
@@ -0,0 +1,387 @@
+ })),
+ fieldPermissions: defaultRole.fieldPermissions.map((permission) => ({
+ __typename: 'FieldPermission' as const,
+ id: uuidv4(),
+ roleId: defaultRole.id,
+ objectMetadataId:
</file context>
...wenty-server/src/engine/core-modules/application/services/mocked-marketplace-app.constant.ts
Outdated
Show resolved
Hide resolved
...wenty-server/src/engine/core-modules/application/services/mocked-marketplace-app.constant.ts
Show resolved
Hide resolved
Greptile OverviewGreptile Summaryadded comprehensive Content and Permissions tabs for marketplace and installed apps in the Twenty CRM settings interface Major changes:
Issues found:
The architecture properly separates concerns between marketplace apps (not yet installed) and installed apps, with the permissions tab intelligently handling both cases. Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant GraphQL
participant MarketplaceService
participant GitHub
User->>Frontend: Navigate to Marketplace Apps
Frontend->>GraphQL: Query findManyMarketplaceApps
GraphQL->>MarketplaceService: findAllMarketplaceApps()
alt Cache Valid
MarketplaceService-->>GraphQL: Return cached apps
else Cache Expired
MarketplaceService->>GitHub: Fetch app directories (API)
GitHub-->>MarketplaceService: Return directory list
loop For each app
MarketplaceService->>GitHub: Fetch manifest.json (raw)
GitHub-->>MarketplaceService: Return manifest
MarketplaceService->>GitHub: Fetch package.json (raw)
GitHub-->>MarketplaceService: Return package.json
MarketplaceService->>MarketplaceService: Parse & build MarketplaceAppDTO
end
MarketplaceService->>MarketplaceService: Cache apps (1 hour TTL)
MarketplaceService-->>GraphQL: Return apps with objects/fields/permissions
end
GraphQL-->>Frontend: Return MarketplaceApp[]
Frontend->>Frontend: Render Content Tab (objects, fields, functions)
User->>Frontend: Click on Permissions Tab
Frontend->>Frontend: resolvePermissionIds (universal→workspace IDs)
Frontend->>Frontend: buildSyntheticRole (create mock role)
Frontend->>Frontend: buildobjectMetadataItemsFromMarketplaceApp
Frontend->>Frontend: Render SettingsRolePermissions
|
| }, | ||
| { | ||
| objectUniversalIdentifier: MOCK_ENRICHMENT_JOB_UNIVERSAL_ID, | ||
| fieldUniversalIdentifier: 'jobTitle', |
There was a problem hiding this comment.
using string 'jobTitle' instead of the constant PERSON_JOB_TITLE_FIELD_UNIVERSAL_ID
| fieldUniversalIdentifier: 'jobTitle', | |
| fieldUniversalIdentifier: PERSON_JOB_TITLE_FIELD_UNIVERSAL_ID, |
| if (!isDefined(objectMetadataItem)) { | ||
| throw new Error( | ||
| `Could not resolve object for universalIdentifier: ${group.objectUniversalIdentifier}`, | ||
| ); |
There was a problem hiding this comment.
throwing an error will crash the UI if the universal identifier cannot be resolved - consider graceful degradation (skip the field group or show a placeholder) instead
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| import { TableRow } from '@/ui/layout/table/components/TableRow'; | ||
|
|
||
| const StyledTableSubRow = styled(TableRow)` | ||
| padding-left: ${({ theme }) => theme.spacing(4)}; |
There was a problem hiding this comment.
prefer direct theme spacing instead of template literals
| padding-left: ${({ theme }) => theme.spacing(4)}; | |
| padding-left: ${({ theme }) => theme.spacing(4)}; |
Context Used: Context from dashboard - When using styled components, prefer to use theme spacing directly in calculations instead of wrappi... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const COMPANY_INDUSTRY_FIELD_UNIVERSAL_ID = | ||
| '20202020-4dc2-47c9-bb15-6e6f19ba9e46'; | ||
| const COMPANY_EMPLOYEE_COUNT_FIELD_UNIVERSAL_ID = | ||
| '20202020-4dc2-47c9-bb15-6e6f19ba9e46'; | ||
| const PERSON_LINKEDIN_URL_FIELD_UNIVERSAL_ID = | ||
| '20202020-4dc2-47c9-bb15-6e6f19ba9e46'; | ||
| const PERSON_JOB_TITLE_FIELD_UNIVERSAL_ID = | ||
| '20202020-4dc2-47c9-bb15-6e6f19ba9e46'; |
There was a problem hiding this comment.
all four field constants share identical universal identifier values, causing field identifier conflicts
| const COMPANY_INDUSTRY_FIELD_UNIVERSAL_ID = | |
| '20202020-4dc2-47c9-bb15-6e6f19ba9e46'; | |
| const COMPANY_EMPLOYEE_COUNT_FIELD_UNIVERSAL_ID = | |
| '20202020-4dc2-47c9-bb15-6e6f19ba9e46'; | |
| const PERSON_LINKEDIN_URL_FIELD_UNIVERSAL_ID = | |
| '20202020-4dc2-47c9-bb15-6e6f19ba9e46'; | |
| const PERSON_JOB_TITLE_FIELD_UNIVERSAL_ID = | |
| '20202020-4dc2-47c9-bb15-6e6f19ba9e46'; | |
| const COMPANY_INDUSTRY_FIELD_UNIVERSAL_ID = | |
| '20202020-ind1-ind1-ind1-industry1111'; | |
| const COMPANY_EMPLOYEE_COUNT_FIELD_UNIVERSAL_ID = | |
| '20202020-emp1-emp1-emp1-employee1111'; | |
| const PERSON_LINKEDIN_URL_FIELD_UNIVERSAL_ID = | |
| '20202020-lin1-lin1-lin1-linkedin1111'; | |
| const PERSON_JOB_TITLE_FIELD_UNIVERSAL_ID = | |
| '20202020-job1-job1-job1-jobtitle1111'; |
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:29657 This environment will automatically shut down when the PR is closed or after 5 hours. |
| @@ -16,6 +16,65 @@ export const FIND_MANY_MARKETPLACE_APPS = gql` | |||
| providers | |||
| websiteUrl | |||
| termsUrl | |||
| objects { | |||
There was a problem hiding this comment.
can't this can be extracted from APPLICATION_FRAGMENT?
{
...marketplaceRelatedFields
...ApplicationFields
}
...ges/twenty-front/src/pages/settings/applications/components/SettingsApplicationDataTable.tsx
Outdated
Show resolved
Hide resolved
| @Field(() => [String]) | ||
| permissionFlags: string[]; | ||
| } | ||
|
|
||
| @ObjectType('MarketplaceApp') | ||
| export class MarketplaceAppDTO { |
There was a problem hiding this comment.
i guess you might extend a lot from ApplicationDTO isn't it?
There was a problem hiding this comment.
I wouldn't say extend as there ApplicationDTO stuff that marketplaceApplicationDTO don't have: yarnlock and packageJson files and checksums, etc.
Their objects and fields also are different; marketplaceApplicationDTO don't have ids.
I think it's safe to keep two different types
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
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-server/src/engine/core-modules/application/services/mocked-marketplace-app.constant.ts">
<violation number="1" location="packages/twenty-server/src/engine/core-modules/application/services/mocked-marketplace-app.constant.ts:197">
P2: Field permission references the Person jobTitle field while the object is Enrichment Job, so the permission won’t match any field on that object. Align the objectUniversalIdentifier with the field’s actual object (Person) or use an enrichment job field identifier instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...wenty-server/src/engine/core-modules/application/services/mocked-marketplace-app.constant.ts
Show resolved
Hide resolved
| const hasFields = isDefined(row.fields) && row.fields.length > 0; | ||
|
|
||
| return ( | ||
| <div> |
There was a problem hiding this comment.
why do you need div here? If you absolutelly need it, you need to use an empty
There was a problem hiding this comment.
needed because there is another element at the same level as TableRow
There was a problem hiding this comment.
in that case you might just need <> </>
In this PR
There are breaking changes but it's behind a feature flag not exposed (access to marketplace) so not problematic
marketplace apps
https://github.com/user-attachments/assets/4c660101-50fc-47ce-b90a-8d6f17db5e74
installed apps
https://github.com/user-attachments/assets/c9229ee1-e75f-4cad-8766-758b2c5b37b4