-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Last email interaction extension #15511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR implements a Twenty extension that automatically tracks email interactions by updating lastInteraction and interactionStatus fields on Person and Company records when messages are created or updated.
Key Changes:
- Added new extension package with Yarn 4.9.2 configuration
- Implemented serverless function that triggers on
message.createdandmessage.updatedevents - Automatically creates custom fields (
lastInteractionandinteractionStatus) if they don't exist - Calculates interaction status based on time since last email (Recent: <7 days, Active: <30 days, Cooling: <90 days, Dormant: >=90 days)
- Updates all related people and companies from message participants
Critical Issues Found:
- Hardcoded API key exposed in
package.json(security risk) - Syntax error: trailing comma in options array
- Incorrect data structure:
interactionStatussent as array instead of string - Field configuration mismatch:
isNullable: falsewithdefaultValue: null - Multiple sequential API calls without proper rate limiting protection
Confidence Score: 0/5
- This PR cannot be merged safely due to multiple critical issues including exposed credentials, syntax errors, and logic bugs
- Score reflects critical security issue (hardcoded API key), syntax error that will prevent execution, and multiple logic bugs that will cause runtime failures
- Pay critical attention to
package.json(exposed API key) andserverlessFunctions/test/src/index.ts(multiple syntax and logic errors)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| packages/twenty-apps/last_email_interaction/package.json | 1/5 | Contains hardcoded API key that must be removed before merging |
| packages/twenty-apps/last_email_interaction/serverlessFunctions/test/src/index.ts | 1/5 | Multiple critical issues: syntax error (trailing comma), incorrect data structure (array instead of string), isNullable/defaultValue mismatch, and rate limiting concerns |
Sequence Diagram
sequenceDiagram
participant Message as Message Event
participant Function as Serverless Function
participant API as Twenty REST API
participant DB as Database
Message->>Function: message.created/updated
Function->>API: GET /metadata/objects
API-->>Function: Return objects metadata
alt Fields don't exist
Function->>API: POST /metadata/fields (lastInteraction)
API-->>DB: Create field
Function->>API: POST /metadata/fields (interactionStatus)
API-->>DB: Create field
end
Function->>API: GET /messages/{recordId}?depth=1
API-->>Function: Return message with participants
loop For each participant
Function->>API: GET /people/{id}
API-->>Function: Return person with companyId
end
loop For each person
Function->>API: PATCH /people/{id}
API-->>DB: Update lastInteraction & interactionStatus
end
loop For each company
Function->>API: PATCH /companies/{id}
API-->>DB: Update lastInteraction & interactionStatus
end
6 files reviewed, 8 comments
packages/twenty-apps/last_email_interaction/serverlessFunctions/test/src/index.ts
Outdated
Show resolved
Hide resolved
packages/twenty-apps/last_email_interaction/serverlessFunctions/test/src/index.ts
Outdated
Show resolved
Hide resolved
packages/twenty-apps/last_email_interaction/serverlessFunctions/test/src/index.ts
Outdated
Show resolved
Hide resolved
packages/twenty-apps/last_email_interaction/serverlessFunctions/test/src/index.ts
Outdated
Show resolved
Hide resolved
| for (const id of peopleIds) { | ||
| const options = { | ||
| method: 'GET', | ||
| url: `${TWENTY_URL}/people/${id}`, | ||
| headers: { | ||
| Authorization: `Bearer ${TWENTY_API_KEY}` | ||
| } | ||
| }; | ||
| const req = await axios.request(options); | ||
| companiesIds.push(req.data.person.companyId); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Multiple sequential API calls without delay between them will cause rate limiting issues. Add delays or use Promise.all with batch processing
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-apps/last_email_interaction/serverlessFunctions/test/src/index.ts
Line: 173:183
Comment:
**logic:** Multiple sequential API calls without delay between them will cause rate limiting issues. Add delays or use Promise.all with batch processing
How can I resolve this? If you propose a fix, please make it concise.| for (const id of peopleIds) { | ||
| const peopleOptions = { | ||
| method: 'PATCH', | ||
| url: `${TWENTY_URL}/people/${id}`, | ||
| headers: { | ||
| 'Content-Type': "application/json", | ||
| Authorization: `Bearer ${TWENTY_API_KEY}` | ||
| }, | ||
| data: interactionData(messageDate, interactionStatus) | ||
| } | ||
| const req = await axios.request(peopleOptions); | ||
| if (req.status === 200) { | ||
| console.log(`Successfully updated person with ID ${id}`); | ||
| await setTimeout(DELAY); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Sequential updates without proper error handling. If one update fails, subsequent updates won't execute
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-apps/last_email_interaction/serverlessFunctions/test/src/index.ts
Line: 185:200
Comment:
**style:** Sequential updates without proper error handling. If one update fails, subsequent updates won't execute
How can I resolve this? If you propose a fix, please make it concise.
packages/twenty-apps/last_email_interaction/serverlessFunctions/test/src/index.ts
Outdated
Show resolved
Hide resolved
| }, | ||
| "TWENTY_API_URL": { | ||
| "isSecret": false, | ||
| "value": "", | ||
| "description": "Optional, defaults to cloud API URL" | ||
| } | ||
| }, | ||
| "dependencies": { | ||
| "axios": "^1.12.2" | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: twenty app sync fails due to missing application.config.ts and twenty-sdk dependency in package.json.
Severity: CRITICAL | Confidence: 0.99
🔍 Detailed Analysis
The twenty app sync command will fail because the extractTwentyAppConfig function, called by loadManifest, unconditionally throws an error if application.config.ts is not found. This file is missing from the last_email_interaction app, preventing successful setup. Users following the documented setup instructions will immediately encounter this error. Additionally, the package.json is missing the twenty-sdk dependency, which is required for type imports.
💡 Suggested Fix
Create application.config.ts with a default ApplicationConfig export. Add "twenty-sdk" to the dependencies in package.json.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/twenty-apps/last_email_interaction/package.json#L1-L29
Potential issue: The `twenty app sync` command will fail because the
`extractTwentyAppConfig` function, called by `loadManifest`, unconditionally throws an
error if `application.config.ts` is not found. This file is missing from the
`last_email_interaction` app, preventing successful setup. Users following the
documented setup instructions will immediately encounter this error. Additionally, the
`package.json` is missing the `twenty-sdk` dependency, which is required for type
imports.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Implements a serverless function that tracks last email interactions by creating custom fields (lastInteraction and interactionStatus) on person and company objects, then updates them based on email receivedAt timestamps.
Key Changes:
- Added README with setup instructions
- Created
package.jsonmanifest with environment configuration - Implemented main serverless function that:
- Checks/creates required custom fields dynamically
- Calculates interaction status (RECENT/ACTIVE/COOLING/DORMANT) based on time since last email
- Fetches message participants and their associated companies
- Updates person and company records with interaction data
Critical Issues Found:
- Line 231: Wrong variable
optionsused instead ofpeopleOptions- will cause runtime error - Line 210: Null
companyIdvalues not filtered, causing failed PATCH requests - Multiple syntax errors from previous review still present (trailing comma, array instead of string)
Confidence Score: 0/5
- This PR cannot be merged - it contains critical bugs that will cause runtime failures
- Multiple critical logic errors exist: using undefined variable (line 231), missing null checks for companyId (line 210), and unresolved syntax errors from previous review. These will cause immediate runtime failures when the function executes.
- packages/twenty-apps/last_email_interaction/serverlessFunctions/test/src/index.ts requires immediate attention - contains multiple critical bugs that prevent execution
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| packages/twenty-apps/last_email_interaction/README.md | 5/5 | Documentation file describing setup and flow. Clear and well-structured with no issues. |
| packages/twenty-apps/last_email_interaction/package.json | 4/5 | Configuration manifest. API key issue resolved (now empty string). Minor version/dependency definitions. |
| packages/twenty-apps/last_email_interaction/serverlessFunctions/test/src/index.ts | 1/5 | Main implementation with critical bugs: wrong variable used (line 231), null companyId handling missing, syntax/logic errors from previous comments unresolved. |
Sequence Diagram
sequenceDiagram
participant Trigger as Webhook Trigger
participant Main as main()
participant API as Twenty REST API
participant Meta as Metadata API
participant Messages as Messages API
participant People as People API
participant Companies as Companies API
Trigger->>Main: New email received (recordId, properties)
Main->>API: GET /metadata/objects
API-->>Main: Return all objects (company, person)
alt Company fields missing
Main->>Meta: POST /metadata/fields (lastInteraction)
Meta-->>Main: Field created
Main->>Meta: POST /metadata/fields (interactionStatus)
Meta-->>Main: Field created
end
alt Person fields missing
Main->>Meta: POST /metadata/fields (lastInteraction)
Meta-->>Main: Field created
Main->>Meta: POST /metadata/fields (interactionStatus)
Meta-->>Main: Field created
end
Main->>Main: Calculate interaction status from receivedAt
Main->>Messages: GET /messages/{recordId}?depth=1
Messages-->>Main: Message with participants
loop For each participant
Main->>People: GET /people/{personId}
People-->>Main: Person with companyId
Main->>Main: Collect companyId (may be null)
end
loop For each person
Main->>People: PATCH /people/{personId} (BUG: uses wrong variable)
People-->>Main: Updated
end
loop For each company (including nulls)
Main->>Companies: PATCH /companies/{companyId}
Companies-->>Main: Updated or Error
end
Main-->>Trigger: Return {}
3 files reviewed, 2 comments
| data: interactionData(messageDate, interactionStatus), | ||
| }; | ||
| try { | ||
| const response = await axios.request(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Wrong variable used. Should be peopleOptions not options
| const response = await axios.request(options); | |
| const response = await axios.request(peopleOptions); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-apps/last_email_interaction/serverlessFunctions/test/src/index.ts
Line: 231:231
Comment:
**logic:** Wrong variable used. Should be `peopleOptions` not `options`
```suggestion
const response = await axios.request(peopleOptions);
```
How can I resolve this? If you propose a fix, please make it concise.| }; | ||
| try { | ||
| const req = await axios.request(options); | ||
| companiesIds.push(req.data.person.companyId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: companyId can be null/undefined if person not linked to company. Will cause issues when updating companies
| companiesIds.push(req.data.person.companyId); | |
| if (req.data.person.companyId) { | |
| companiesIds.push(req.data.person.companyId); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-apps/last_email_interaction/serverlessFunctions/test/src/index.ts
Line: 210:210
Comment:
**logic:** `companyId` can be null/undefined if person not linked to company. Will cause issues when updating companies
```suggestion
if (req.data.person.companyId) {
companiesIds.push(req.data.person.companyId);
}
```
How can I resolve this? If you propose a fix, please make it concise.
martmull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thank you for your submission!
Code looks nice, did not test it yet
Some comments if you want to take a look
Cheers
| }, | ||
| "TWENTY_API_URL": { | ||
| "isSecret": false, | ||
| "value": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add default value to "value"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default you mean "default":"{value}" or "value":"{value}"? Also, is it necessary when in first lines of code I'm checking if TWENTY_API_URL is defined and if not, it defaults to cloud API URL
| if (response2.status === 201) { | ||
| console.log('Successfully created person interaction status field'); | ||
| } | ||
| await setTimeout(DELAY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need await setTimeout(DELAY); everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original idea was to just add a little bit of delay between requests to prevent accidentally DDoSing workspace but I think something else should be done if it's necessary
|
@martmull thanks for review, left questions to comments as I'm not entirely sure about them |
Challenge 4 from "Call for projects" list