Connectors | Add getConnection method - for getting access token and connection config

[davidvaks]

No description provided.

[claude]

Claude finished @davidvaks's task —— View job

---

Code Review for PR #135: Add getConnection method

Code Review Tasks

• Read the modified files to understand the changes
• Check repo guidelines (CLAUDE.md)
• Review code quality and best practices
• Identify potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Check test coverage
• Provide comprehensive feedback

Review Summary

This PR adds a new getConnection method to the connectors module that returns both the access token and connection configuration. While the implementation is functional, there are several important issues that need to be addressed.

🔴 Critical Issues

1. Type Safety Problems with getAccessToken

The existing getAccessToken method has significant type issues:

• The interface declares it returns Promise<string> (src/modules/connectors.types.ts:101)
• The implementation returns response.access_token which is actually the full response object (src/modules/connectors.ts:36)
• There are @ts-expect-error comments suppressing these type errors (lines 23, 36)

This is a breaking bug that needs to be fixed. The method should return just the token string:

return response.data.access_token;

2. Incorrect Response Handling

Both methods make incorrect assumptions about the Axios response structure:

In getAccessToken (line 31-36):

const response = await axios.get<ConnectorAccessTokenResponse>(
  `/apps/${appId}/external-auth/tokens/${integrationType}`
);
return response.access_token; // Wrong! Should be response.data.access_token

In getConnection (line 46-50):

const response = await axios.get<ConnectorAccessTokenResponse>(
  `/apps/${appId}/external-auth/tokens/${integrationType}`
);
const data = response as unknown as ConnectorAccessTokenResponse; // Unnecessary casting

The correct pattern is to access response.data as Axios returns the parsed response in the data property.

3. No Error Handling

Neither method handles potential API errors:

• 404 when connector doesn't exist
• 401/403 for authentication issues
• Network errors
• Invalid response formats

🟡 Code Quality Issues

1. Code Duplication

Both methods duplicate the same validation and API call logic. Consider extracting a shared helper:

private async fetchConnectorToken(integrationType: ConnectorIntegrationType): Promise<ConnectorAccessTokenResponse> {
  if (!integrationType || typeof integrationType !== "string") {
    throw new Error("Integration type is required and must be a string");
  }
  
  const response = await axios.get<ConnectorAccessTokenResponse>(
    `/apps/${appId}/external-auth/tokens/${integrationType}`
  );
  
  return response.data;
}

2. Inconsistent Naming

The API endpoint returns connection_config (snake_case) but the interface uses connectionConfig (camelCase). While the camelCase conversion is good for the SDK interface, this transformation should be explicit and documented.

🟢 Good Practices

• Clear JSDoc documentation with examples
• Proper TypeScript interfaces with detailed comments
• Consistent with SDK patterns for module structure
• Good use of nullish coalescing for connectionConfig

📋 Additional Recommendations

1. Add Unit Tests

No tests exist for the connectors module. Add tests covering:

• Successful token retrieval
• Error scenarios (404, 401, network errors)
• Input validation
• Type correctness

2. Add PR Description

The PR lacks a description explaining:

• Why this change is needed
• What use cases it enables
• Any breaking changes or migration notes

3. Consider Response Caching

OAuth tokens often have long lifetimes. Consider caching responses to reduce API calls, especially if tokens are requested frequently.

4. Security Enhancement

Consider adding options to:

• Mask tokens in debug logs
• Add token expiry information if available from the API

🔧 Suggested Implementation

Here's how the module should be refactored:

export function createConnectorsModule(
  axios: AxiosInstance,
  appId: string
): ConnectorsModule {
  async function fetchConnectorData(
    integrationType: ConnectorIntegrationType
  ): Promise<ConnectorAccessTokenResponse> {
    if (!integrationType || typeof integrationType !== "string") {
      throw new Error("Integration type is required and must be a string");
    }

    try {
      const response = await axios.get<ConnectorAccessTokenResponse>(
        `/apps/${appId}/external-auth/tokens/${integrationType}`
      );
      return response.data;
    } catch (error) {
      if (axios.isAxiosError(error)) {
        if (error.response?.status === 404) {
          throw new Error(`Connector '${integrationType}' not found or not configured`);
        }
        throw new Error(`Failed to fetch connector token: ${error.message}`);
      }
      throw error;
    }
  }

  return {
    async getAccessToken(integrationType: ConnectorIntegrationType): Promise<string> {
      const data = await fetchConnectorData(integrationType);
      return data.access_token;
    },

    async getConnection(
      integrationType: ConnectorIntegrationType
    ): Promise<ConnectorConnectionResponse> {
      const data = await fetchConnectorData(integrationType);
      return {
        accessToken: data.access_token,
        connectionConfig: data.connection_config ?? null,
      };
    },
  };
}

Conclusion

While the new getConnection method adds useful functionality, the implementation has critical bugs that must be fixed before merging. The existing getAccessToken method is also broken and needs immediate attention. Please address the type safety issues, fix the Axios response handling, and add proper error handling before this can be approved.

[github-actions]

🚀 Package Preview Available!

---

Install this PR's preview build with npm:

npm i @base44-preview/sdk@0.8.19-pr.135.721e301

Prefer not to change any import paths? Install using npm alias so your code still imports @base44/sdk:

npm i "@base44/sdk@npm:@base44-preview/sdk@0.8.19-pr.135.721e301"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "@base44/sdk": "npm:@base44-preview/sdk@0.8.19-pr.135.721e301"
  }
}

• 📦 Preview Package: @base44-preview/sdk@0.8.19-pr.135.721e301
• 🔗 View this commit on GitHub

---

_{Preview published to npm registry — try new features instantly!}