Skip to content

followup 18044#18213

Merged
charlesBochet merged 8 commits intomainfrom
refact-client-service
Feb 25, 2026
Merged

followup 18044#18213
charlesBochet merged 8 commits intomainfrom
refact-client-service

Conversation

@ehconitin
Copy link
Copy Markdown
Contributor

No description provided.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Feb 25, 2026

Greptile Summary

This PR refactors the Twenty SDK client service by extracting the inline Twenty client template code into a separate file (twenty-client-template.ts).

Changes:

  • Extracted 400+ line template string from client-service.ts into twenty-client-template.ts
  • Cleaned up imports in client-service.ts by removing unused constants
  • Maintains same functionality as PR Host-remote refresh token implementation #18044, just better organized

Code quality suggestions:

  • Consider using the isNonEmptyString utility function in several places in the template for consistency with codebase standards

Confidence Score: 5/5

  • This PR is safe to merge - it's a pure refactoring with no logic changes
  • The refactoring extracts a large inline template string into a separate file with zero logic changes. The code is moved verbatim, maintaining identical functionality. Only minor style improvements suggested.
  • No files require special attention

Important Files Changed

Filename Overview
packages/twenty-sdk/src/cli/utilities/client/client-service.ts Extracted inline template string to separate file, removed unused imports
packages/twenty-sdk/src/cli/utilities/client/twenty-client-template.ts New file containing Twenty client template code, extracted from client-service.ts

Last reviewed commit: 84187a5

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/twenty-sdk/src/cli/utilities/client/twenty-client-template.ts">

<violation number="1" location="packages/twenty-sdk/src/cli/utilities/client/twenty-client-template.ts:407">
P1: `setAuthorizationToken` overwrites `API_KEY_ENV_KEY` with a refreshed access token, corrupting a different credential type. An API key (`TWENTY_API_KEY`) is a static long-lived credential and should not be overwritten with an ephemeral access token. Only `APP_ACCESS_TOKEN_ENV_KEY` should be updated on token refresh.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Made-with: Cursor

# Conflicts:
#	packages/twenty-sdk/src/cli/utilities/client/client-service.ts
Comment on lines +64 to +66
expect(mockCreateOneNoteTarget).toHaveBeenCalledWith({
noteId: fakeNoteId,
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The setAuthorizationToken method no longer updates the legacy API_KEY_ENV_KEY environment variable, which can cause authentication failures for integrations relying on it.
Severity: CRITICAL

Suggested Fix

In the setAuthorizationToken method within packages/twenty-sdk/src/cli/utilities/client/twenty-client-template.ts, re-add the line processEnvironment[API_KEY_ENV_KEY] = token; to ensure backward compatibility by updating both the new and legacy environment variables with the refreshed token.

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-front/src/modules/activities/hooks/__tests__/useOpenCreateActivityDrawer.test.tsx#L64-L66

Potential issue: The refactored `setAuthorizationToken` method in
`twenty-client-template.ts` no longer updates the `API_KEY_ENV_KEY` environment variable
upon token refresh. The previous implementation updated both `APP_ACCESS_TOKEN_ENV_KEY`
and `API_KEY_ENV_KEY` to maintain backward compatibility. The removal of the
`API_KEY_ENV_KEY` update means that any legacy integrations or parts of the system that
still rely on this variable will not receive the refreshed authentication token. This
will lead to authentication failures for those systems after a token refresh event.

@charlesBochet charlesBochet merged commit a9649b0 into main Feb 25, 2026
68 of 73 checks passed
@charlesBochet charlesBochet deleted the refact-client-service branch February 25, 2026 23:55
@twenty-eng-sync
Copy link
Copy Markdown

Hey @ehconitin! After you've done the QA of your Pull Request, you can mark it as done here. Thank you!

@github-actions
Copy link
Copy Markdown
Contributor

Thanks @ehconitin for your contribution!
This marks your 325th PR on the repo. You're top 1% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants