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.0277f81

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.0277f81"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "@base44/sdk": "npm:@base44-preview/sdk@0.8.19-pr.135.0277f81"
  }
}

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

---

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

[claude]

Claude finished @davidvaks's task —— View job

---

Code Review for PR #135: Connectors - Add getConnection method

Review Tasks

• Read the modified files to understand the changes
• Check for repository guidelines (CLAUDE.md)
• Analyze code quality and best practices
• Check if previous review issues were addressed
• Identify potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Check test coverage
• Provide comprehensive feedback

Summary

This PR adds a deprecation notice to getAccessToken and introduces a new getConnection method. Unfortunately, none of the critical issues from the previous review have been addressed, and the implementation still contains significant bugs.

🔴 Critical Issues Not Addressed

1. Broken Response Handling

The most severe issue remains unfixed. Both methods incorrectly access the Axios response:

In getAccessToken (line 42):

return response.access_token; // ❌ Wrong - should be response.data.access_token

In getConnection (lines 52-60):

const response = await axios.get<ConnectorAccessTokenResponse>(...);
const data = response as unknown as ConnectorAccessTokenResponse; // ❌ Unnecessary cast
return {
  accessToken: data.access_token,       // ❌ This accesses response.access_token
  connectionConfig: data.connection_config ?? null,
};

Both methods are accessing response.access_token when they should access response.data.access_token. Axios returns the parsed response in the data property.

2. Type Safety Violations

The @ts-expect-error comments (lines 26, 41) are suppressing legitimate TypeScript errors:

• Line 26: The function signature doesn't match the interface definition
• Line 41: The return type is wrong due to the incorrect response handling

3. No Error Handling

Neither method handles API errors, which could lead to unhelpful error messages for:

• 404 - Connector not found
• 401/403 - Authentication errors
• Network failures
• Invalid response formats

🟡 Issues Partially Addressed

1. Deprecation Added ✅

Good that getAccessToken now includes:

• @deprecated JSDoc tag
• Console warning on usage
• Clear migration guidance

However, the deprecation would be more effective if the method actually worked correctly.

2. Code Duplication ❌

The validation and API call logic is still duplicated between both methods.

🟢 Good Additions

• Clear JSDoc documentation with examples
• Proper TypeScript interfaces
• Good use of nullish coalescing for connectionConfig
• Helpful code examples in the documentation

📋 Recommendations

1. Fix the Critical Bugs First

// getAccessToken should be:
const response = await axios.get<ConnectorAccessTokenResponse>(...);
return response.data.access_token;

// getConnection should be:
const response = await axios.get<ConnectorAccessTokenResponse>(...);
return {
  accessToken: response.data.access_token,
  connectionConfig: response.data.connection_config ?? null,
};

2. Add Error Handling

try {
  const response = await axios.get<ConnectorAccessTokenResponse>(...);
  return response.data;
} catch (error) {
  if (axios.isAxiosError(error)) {
    if (error.response?.status === 404) {
      throw new Error(`Connector '${integrationType}' not found`);
    }
    // Handle other HTTP errors
  }
  throw error;
}

3. Add Tests

The connectors module has zero test coverage. Critical functionality like this needs comprehensive testing.

4. Consider a Shared Helper

To eliminate duplication:

private async fetchConnectorData(
  integrationType: ConnectorIntegrationType
): Promise<ConnectorAccessTokenResponse> {
  // Validation + error handling + API call
}

Conclusion

This PR cannot be merged in its current state. The implementation has critical bugs that would cause runtime errors for anyone trying to use these methods. Please fix the Axios response handling issue as a minimum before this can be considered for merging.

[claude]

Claude finished @davidvaks's task —— View job

---

Code Review for PR #135: Connectors - Add getConnection method

Review Tasks

• Read the modified files to understand the changes
• Check for repository guidelines (CLAUDE.md)
• Analyze if previous review issues were addressed
• Identify any new issues or concerns
• Evaluate code quality and best practices
• Check test coverage
• Provide comprehensive feedback

Summary

This PR adds a new getConnection method to the connectors module and deprecates the existing getAccessToken method. The latest commit removed the console warning from the deprecated method. Critical issues from previous reviews remain unfixed, making this PR unsuitable for merging.

🔴 Critical Bugs - Must Fix

1. Broken Axios Response Handling

Both methods incorrectly access the Axios response object. This is a runtime error that will break for all users:

In getAccessToken (line 39):

const response = await axios.get<ConnectorAccessTokenResponse>(...);
// @ts-expect-error
return response.access_token; // ❌ WRONG - should be response.data.access_token

In getConnection (lines 49-57):

const response = await axios.get<ConnectorAccessTokenResponse>(...);
const data = response as unknown as ConnectorAccessTokenResponse; // ❌ Unnecessary cast
return {
  accessToken: data.access_token,       // ❌ Still accessing response.access_token
  connectionConfig: data.connection_config ?? null,
};

Fix required:

// getAccessToken:
return response.data.access_token;

// getConnection:
return {
  accessToken: response.data.access_token,
  connectionConfig: response.data.connection_config ?? null,
};

2. Type Safety Violations

The @ts-expect-error comments (lines 26, 38) suppress legitimate TypeScript errors:

• Line 26: Function returns ConnectorAccessTokenResponse but interface expects string
• Line 38: Accessing non-existent property on Axios response

These errors exist because the implementation is incorrect, not because TypeScript is wrong.

🔴 Major Issues

3. No Error Handling

Neither method handles API errors, resulting in poor developer experience:

• 404 errors (connector not found) will show generic Axios errors
• Authentication failures won't provide helpful context
• Network errors aren't caught

Suggested implementation:

try {
  const response = await axios.get<ConnectorAccessTokenResponse>(...);
  return response.data;
} catch (error) {
  if (axios.isAxiosError(error)) {
    if (error.response?.status === 404) {
      throw new Error(`Connector '${integrationType}' not found or not configured`);
    }
    if (error.response?.status === 401) {
      throw new Error(`Authentication failed for connector '${integrationType}'`);
    }
  }
  throw error;
}

4. Zero Test Coverage

The connectors module has no tests. Critical functionality like this needs comprehensive testing covering:

• Successful token retrieval
• Various error scenarios
• Type correctness
• Input validation

🟡 Code Quality Issues

5. Code Duplication

Both methods duplicate validation and API call logic. Extract a shared helper:

private async 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) {
    // Error handling as shown above
  }
}

6. Deprecation Strategy

While the @deprecated JSDoc tag is good, removing the console warning in the latest commit reduces visibility of the deprecation. Consider keeping it to help developers migrate.

🟢 Good Practices

• Excellent JSDoc documentation with clear examples
• Well-structured TypeScript interfaces with detailed comments
• Good use of nullish coalescing for connectionConfig
• Clear camelCase conversion from snake_case API response

📋 Missing PR Context

• No PR description explaining the motivation for this change
• No migration guide for users of getAccessToken
• No explanation of what connection_config contains or when to use it

Conclusion

This PR cannot be merged due to critical runtime errors. The Axios response handling bug will cause all API calls to fail. Please fix at minimum:

1. Change response.access_token to response.data.access_token in both methods
2. Remove the unnecessary type casting in getConnection
3. Remove @ts-expect-error comments after fixing the actual issues

Once these critical bugs are fixed, consider addressing the error handling and test coverage before merging.

[claude]

Claude finished @davidvaks's task —— View job

---

Code Review for PR #135: Connectors - Add getConnection method

Review Tasks

• Read the modified files to understand the latest changes
• Check if previous critical issues were addressed
• Review the new test file for coverage and quality
• Analyze code quality and best practices
• Identify any remaining or new issues
• Provide comprehensive feedback

Summary

This PR adds a new getConnection method to the connectors module and deprecates the existing getAccessToken method. After analyzing the code and the SDK's axios interceptor configuration, I need to correct my previous reviews - the implementation is actually correct. The axios client has a response interceptor that returns response.data instead of the full response object.

✅ Previous Issues Resolved

1. Axios Response Handling - Actually Correct

My previous reviews incorrectly identified this as a bug. The SDK's axios client configuration includes an interceptor (src/utils/axios-client.ts:229) that transforms responses to return response.data directly. This means:

• response.access_token is correct (not response.data.access_token)
• The implementation works as intended with the SDK's architecture

2. Tests Added

Good test coverage has been added for the getConnection method, covering:

• Successful token retrieval with connection config
• Handling missing connection config (returns null)
• Input validation (empty string, null values)

🟡 Remaining Issues

1. Type Safety Concerns

The @ts-expect-error comments (lines 26, 38) are still present:

• Line 26: Function implementation doesn't match interface - the return type is declared as Promise<string> in the interface but the implementation returns Promise<ConnectorAccessTokenResponse>
• Line 38: TypeScript correctly identifies that response.access_token doesn't exist on the Axios response type

Recommendation: Update the implementation to match the interface:

async getAccessToken(
  integrationType: ConnectorIntegrationType
): Promise<string> {  // Match interface type
  // ... validation ...
  const response = await axios.get<ConnectorAccessTokenResponse>(...);
  return response.access_token; // Now correctly typed
}

2. Code Duplication

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

private async fetchConnectorData(
  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;
}

3. No Error Handling

Neither method handles API errors gracefully. Users will see generic Axios errors instead of helpful messages:

try {
  const response = await axios.get<ConnectorAccessTokenResponse>(...);
  return response;
} catch (error) {
  if (axios.isAxiosError(error)) {
    if (error.response?.status === 404) {
      throw new Error(`Connector '${integrationType}' not found or not configured`);
    }
    // Handle other HTTP errors
  }
  throw error;
}

4. Test Coverage Gaps

While basic tests exist, consider adding:

• Tests for getAccessToken method
• Error scenario tests (404, 401, network errors)
• Tests verifying the deprecation warning behavior

5. Unnecessary Type Casting

In getConnection (line 53), the type casting is unnecessary:

const data = response as unknown as ConnectorAccessTokenResponse; // Not needed
// Since response is already the data due to interceptor

🟢 Good Practices

• Excellent JSDoc documentation with clear examples
• Well-structured TypeScript interfaces with detailed comments
• Proper deprecation notice with migration guidance
• Good test structure using Vitest and nock
• Clear separation of response types (snake_case API to camelCase SDK)

📋 Recommendations

1. Remove @ts-expect-error comments by fixing the actual type issues
2. Add comprehensive error handling for better developer experience
3. Extract shared logic to reduce code duplication
4. Expand test coverage to include error scenarios and the deprecated method
5. Add PR description explaining the motivation and migration path

Conclusion

The implementation is functionally correct given the SDK's architecture with axios interceptors. The main concerns are around type safety, error handling, and code quality. While these aren't blocking issues, addressing them would improve the developer experience and maintainability of the code.