Skip to content
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

feat: airtable integration #339

Closed

Conversation

AmitChauhan63390
Copy link
Contributor

@AmitChauhan63390 AmitChauhan63390 commented Jan 11, 2025

Summary by CodeRabbit

  • New Features
    • Added Airtable integration support to the application.
    • Introduced optional Airtable configuration properties including base ID, table name, personal access token, and access token.
    • Enhanced the IntegrationSettings component to manage Airtable-specific settings and authentication.
    • Implemented functionality for updating Airtable records based on robot run outcomes.
    • Enabled concurrent processing of updates for both Google Sheets and Airtable.
    • Added a new development configuration for PostgreSQL database connection.

Copy link

coderabbitai bot commented Jan 11, 2025

Walkthrough

The pull request introduces several new optional properties to the Robot model in the server-side TypeScript code for Airtable integration. These properties include identifiers and tokens necessary for connecting to Airtable. The RobotAttributes interface and Robot class are updated accordingly, and the model initialization is modified to accommodate these new fields. Additionally, a new development configuration for PostgreSQL is added, alongside migration scripts for updating the database schema and enhancements to the workflow management system for processing Airtable updates.

Changes

File Change Summary
server/src/models/Robot.ts - Added optional Airtable-related properties to RobotAttributes interface
- Updated Robot class with new Airtable fields
- Modified Robot.init() to include new nullable Airtable connection fields
- Changed google_sheet_name and google_sheet_id from optional to required
config/config.json - Added new development configuration for PostgreSQL database connection
migrations/20250111140925-add-airtable-fields-to-robot.js - Added migration methods to introduce new columns for Airtable integration in the 'robot' table
package.json - Added new dependency for Airtable package
server/src/workflow-management/integrations/airtableintegration.ts - Introduced functionality for updating Airtable records and managing update tasks
- Added new interface and functions for Airtable integration
server/src/workflow-management/scheduler/index.ts - Integrated Airtable update tasks into the existing workflow management system
src/components/integration/IntegrationSettings.tsx - Added support for Airtable integration in the settings component, including new fields and authentication functions

Possibly related PRs

Suggested labels

Type: Enhancement

Suggested reviewers

  • amhsirak

Poem

🤖 A Robot with Airtable dreams so bright,
Connecting data with magical might!
Base IDs and tokens now in its core,
Integrations it will soon explore!
A digital rabbit's technological delight! 🚀


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🔭 Outside diff range comments (1)
server/src/models/Robot.ts (1)

Airtable integration implementation is incomplete

The model changes add Airtable fields but lack essential implementation components that are present in the existing GSheet integration:

  • Missing server/src/workflow-management/integrations/airtable.ts with:
    • Airtable API client setup and authentication
    • Data transformation and writing logic
    • Update task queue with retry mechanism
    • Error handling for API operations

Reference the existing GSheet integration at server/src/workflow-management/integrations/gsheet.ts for the established integration pattern.

🔗 Analysis chain

Line range hint 28-121: Verify integration implementation completeness

While the model changes lay the groundwork for Airtable integration, there might be additional components needed:

  1. Airtable API client/service
  2. Integration configuration validation
  3. Error handling for Airtable operations
  4. Migration script for adding these columns

Would you like me to help create a checklist of remaining implementation tasks?

Let's check for any related integration code:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for Airtable-related implementation files
rg -l "airtable" --type ts --glob '!{server/src/models/Robot.ts}'

# Check for any existing integration patterns
rg -l "integration|client|service" --type ts --glob 'server/src/**'

Length of output: 629


Script:

#!/bin/bash
# Check GSheet integration implementation
cat server/src/workflow-management/integrations/gsheet.ts

# Look for integration interfaces/base classes
ast-grep --pattern 'interface $_Integration {
  $$$
}'

ast-grep --pattern 'class $_Integration {
  $$$
}'

Length of output: 5748

🧹 Nitpick comments (2)
server/src/models/Robot.ts (2)

106-121: Consider database optimizations

A few suggestions for the column definitions:

  1. Add length constraints to the STRING fields to optimize storage
  2. Consider adding indexes if these fields will be used in queries
  3. Add validate constraints if specific formats are expected (e.g., for base_id)

Example:

 airtable_base_id: {
   type: DataTypes.STRING,
   allowNull: true,
+  length: 100,
+  validate: {
+    // Add format validation if needed
+  }
 },

134-140: Document the relationship requirements

The commented-out relationship code lacks context. If this is planned for future implementation:

  1. Add a TODO comment explaining when/why this will be needed
  2. Create a tracking issue for implementing these relationships
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 56e2ced and 31aaefa.

📒 Files selected for processing (1)
  • server/src/models/Robot.ts (5 hunks)

Comment on lines 60 to 63
public airtable_base_id!: string | null;
public airtable_table_name!: string | null;
public airtable_api_key!: string | null;
public airtable_access_token!: string | null;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove unnecessary non-null assertions

The properties are declared with non-null assertions (!) despite being nullable (string | null). This could lead to runtime errors. Consider:

-  public airtable_base_id!: string | null;
-  public airtable_table_name!: string | null;
-  public airtable_api_key!: string | null;
-  public airtable_access_token!: string | null;
+  public airtable_base_id?: string | null;
+  public airtable_table_name?: string | null;
+  public airtable_api_key?: string | null;
+  public airtable_access_token?: string | null;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public airtable_base_id!: string | null;
public airtable_table_name!: string | null;
public airtable_api_key!: string | null;
public airtable_access_token!: string | null;
public airtable_base_id?: string | null;
public airtable_table_name?: string | null;
public airtable_api_key?: string | null;
public airtable_access_token?: string | null;

Comment on lines 28 to 31
airtable_base_id?: string | null; // Airtable Base ID
airtable_table_name?: string | null; // Airtable Table Name
airtable_api_key?: string | null; // Airtable API Key
airtable_access_token?: string | null; // Airtable OAuth Access Token
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Implement encryption for Airtable credentials using existing utils

The codebase has robust encryption utilities in server/src/utils/auth.ts that are already used for similar sensitive data. Use the encrypt()/decrypt() functions to secure the Airtable credentials before storing them, similar to how proxy credentials are handled.

  • In Robot model operations, encrypt values before saving to database
  • Decrypt values when reading for Airtable API calls
    [security]
🔗 Analysis chain

Consider security measures for Airtable credentials

The Airtable API key and access token are sensitive credentials. Consider:

  1. Encrypting these fields at rest in the database
  2. Using environment variables or a secure credential management system
  3. Implementing proper access controls and audit logging for these fields

Let's check if there are any encryption utilities or secure credential handling patterns in the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for encryption-related code or secure credential handling
rg -i "encrypt|decrypt|credentials|secret" --type ts

Length of output: 9606

@amhsirak amhsirak added the Type: Feature New features label Jan 12, 2025
@amhsirak amhsirak added the Type: Integration All issues/PRs related to integrations label Jan 14, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (5)
server/src/workflow-management/integrations/airtableintegration.ts (2)

36-36: Simplify condition using optional chaining.

You can simplify the condition by using optional chaining to make the code cleaner and reduce potential errors when accessing nested properties.

Apply this diff:

-      } else if (plainRun.binaryOutput && plainRun.binaryOutput['item-0']) {
+      } else if (plainRun.binaryOutput?.['item-0']) {
🧰 Tools
🪛 Biome (1.9.4)

[error] 36-36: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


48-53: Ensure secure handling of sensitive credentials.

The personalAccessToken is used directly from the plainRobot object. For enhanced security, consider encrypting sensitive credentials like API keys and access tokens in storage and decrypt them only when needed.

src/components/integration/IntegrationSettings.tsx (2)

Line range hint 18-18: Remove unused import of 'js-cookie'.

The js-cookie library is imported but not used since custom helper functions are defined to handle cookies. Consider removing the import to clean up the code.

Apply this diff:

-import Cookies from 'js-cookie';

351-351: Simplify conditional rendering using optional chaining.

You can simplify the condition by using optional chaining, enhancing code readability.

Apply this diff:

-            {recording && recording.airtable_base_id ? (
+            {recording?.airtable_base_id ? (
🧰 Tools
🪛 Biome (1.9.4)

[error] 351-351: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

migrations/20250111140925-add-airtable-fields-to-robot.js (1)

1-1: Remove redundant 'use strict' directive.

Modules in ECMAScript are in strict mode by default. The 'use strict'; directive is unnecessary and can be removed.

Apply this diff:

-'use strict';
🧰 Tools
🪛 Biome (1.9.4)

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 31aaefa and 722b97e.

📒 Files selected for processing (7)
  • config/config.json (1 hunks)
  • migrations/20250111140925-add-airtable-fields-to-robot.js (1 hunks)
  • package.json (1 hunks)
  • server/src/models/Robot.ts (4 hunks)
  • server/src/workflow-management/integrations/airtableintegration.ts (1 hunks)
  • server/src/workflow-management/scheduler/index.ts (3 hunks)
  • src/components/integration/IntegrationSettings.tsx (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • server/src/models/Robot.ts
🧰 Additional context used
🪛 Biome (1.9.4)
migrations/20250111140925-add-airtable-fields-to-robot.js

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

src/components/integration/IntegrationSettings.tsx

[error] 351-351: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

server/src/workflow-management/integrations/airtableintegration.ts

[error] 36-36: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (4)
server/src/workflow-management/integrations/airtableintegration.ts (1)

79-79: Verify Airtable SDK support for personal access tokens.

Initializing Airtable with apiKey: personalAccessToken may not be suitable for personal access tokens. The Airtable SDK might require a different method for authentication when using personal access tokens instead of API keys. Ensure that the SDK version supports personal access tokens or adjust the code accordingly.

Please run the following script to check if the Airtable SDK supports personal access tokens:

server/src/workflow-management/scheduler/index.ts (1)

9-9: LGTM!

The import statement for Airtable integration functions is correctly structured.

config/config.json (1)

1-11: 🛠️ Refactor suggestion

Enhance database configuration security and performance.

The current configuration has several areas for improvement:

  1. Using default credentials in development configuration poses a security risk
  2. Missing essential configuration options:
    • SSL mode
    • Connection pool settings
    • Logging options
    • Statement timeout

Consider updating the configuration:

 {
   "development": {
-    "username": "postgres",
-    "password": "postgres",
+    "username": "${DB_USER}",
+    "password": "${DB_PASSWORD}",
     "database": "maxun",
     "host": "localhost",
     "port": 5432,
-    "dialect": "postgres"
+    "dialect": "postgres",
+    "pool": {
+      "max": 5,
+      "min": 0,
+      "acquire": 30000,
+      "idle": 10000
+    },
+    "dialectOptions": {
+      "ssl": false,
+      "statement_timeout": 10000
+    },
+    "logging": true
   }
 }
package.json (1)

24-24: 🛠️ Refactor suggestion

Update Airtable dependency and add type definitions.

The Airtable package configuration needs attention:

  1. Version 0.12.2 is not the latest (current is 0.12.5)
  2. Missing TypeScript type definitions (@types/airtable)

Update the dependencies:

 {
   "dependencies": {
-    "airtable": "^0.12.2",
+    "airtable": "^0.12.5",
   },
   "devDependencies": {
+    "@types/airtable": "^0.10.1",
   }
 }
✅ Verification successful

Confirmed: TypeScript types needed for Airtable integration

The codebase extensively uses TypeScript with Airtable in multiple components, including integration services and React components. Adding @types/airtable will enhance type safety and developer experience. Updating to the latest version is also recommended for consistency.

  • Key TypeScript files using Airtable:
    • server/src/workflow-management/integrations/airtableintegration.ts
    • server/src/models/Robot.ts
    • src/components/integration/IntegrationSettings.tsx
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any existing Airtable type imports
rg "from ['|\"]@types/airtable" 

# Verify if Airtable is used with TypeScript
ast-grep --pattern 'import { $_ } from "airtable"'

Length of output: 88


Script:

#!/bin/bash
# Check for any Airtable usage patterns
rg -i "airtable" --type ts --type js --type json -g '!package*.json'

# Check for TypeScript configuration
fd "tsconfig.json"

Length of output: 11873

Comment on lines +108 to +129
for (const runId in airtableUpdateTasks) {
const task = airtableUpdateTasks[runId];
console.log(`Processing task for runId: ${runId}, status: ${task.status}`);

if (task.status === 'pending') {
hasPendingTasks = true;
try {
await updateAirtable(task.robotId, task.runId);
console.log(`Successfully updated Airtable for runId: ${runId}`);
delete airtableUpdateTasks[runId];
} catch (error: any) {
console.error(`Failed to update Airtable for run ${task.runId}:`, error);
if (task.retries < MAX_RETRIES) {
airtableUpdateTasks[runId].retries += 1;
console.log(`Retrying task for runId: ${runId}, attempt: ${task.retries}`);
} else {
airtableUpdateTasks[runId].status = 'failed';
console.log(`Max retries reached for runId: ${runId}. Marking task as failed.`);
}
}
}
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid modifying airtableUpdateTasks while iterating over it.

Deleting properties from airtableUpdateTasks during iteration can lead to unexpected behavior or skipped entries. This might cause some tasks to be missed during processing.

Refactor the code to collect the tasks to delete and remove them after the iteration. Apply this diff:

 export const processAirtableUpdates = async () => {
   while (true) {
     let hasPendingTasks = false;
+    const tasksToDelete: string[] = [];

     for (const runId in airtableUpdateTasks) {
       const task = airtableUpdateTasks[runId];
       console.log(`Processing task for runId: ${runId}, status: ${task.status}`);

       if (task.status === 'pending') {
         hasPendingTasks = true;
         try {
           await updateAirtable(task.robotId, task.runId);
           console.log(`Successfully updated Airtable for runId: ${runId}`);
-          delete airtableUpdateTasks[runId];
+          tasksToDelete.push(runId);
         } catch (error: any) {
           console.error(`Failed to update Airtable for run ${task.runId}:`, error);
           if (task.retries < MAX_RETRIES) {
             airtableUpdateTasks[runId].retries += 1;
             console.log(`Retrying task for runId: ${runId}, attempt: ${task.retries}`);
           } else {
             airtableUpdateTasks[runId].status = 'failed';
             console.log(`Max retries reached for runId: ${runId}. Marking task as failed.`);
           }
         }
       }
     }

+    // Remove tasks after iteration
+    for (const runId of tasksToDelete) {
+      delete airtableUpdateTasks[runId];
+    }

     if (!hasPendingTasks) {
       console.log('No pending tasks. Exiting loop.');
       break;
     }

     console.log('Waiting for 5 seconds before checking again...');
     await new Promise((resolve) => setTimeout(resolve, 5000));
   }
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for (const runId in airtableUpdateTasks) {
const task = airtableUpdateTasks[runId];
console.log(`Processing task for runId: ${runId}, status: ${task.status}`);
if (task.status === 'pending') {
hasPendingTasks = true;
try {
await updateAirtable(task.robotId, task.runId);
console.log(`Successfully updated Airtable for runId: ${runId}`);
delete airtableUpdateTasks[runId];
} catch (error: any) {
console.error(`Failed to update Airtable for run ${task.runId}:`, error);
if (task.retries < MAX_RETRIES) {
airtableUpdateTasks[runId].retries += 1;
console.log(`Retrying task for runId: ${runId}, attempt: ${task.retries}`);
} else {
airtableUpdateTasks[runId].status = 'failed';
console.log(`Max retries reached for runId: ${runId}. Marking task as failed.`);
}
}
}
}
for (const runId in airtableUpdateTasks) {
const task = airtableUpdateTasks[runId];
console.log(`Processing task for runId: ${runId}, status: ${task.status}`);
if (task.status === 'pending') {
hasPendingTasks = true;
try {
await updateAirtable(task.robotId, task.runId);
console.log(`Successfully updated Airtable for runId: ${runId}`);
tasksToDelete.push(runId);
} catch (error: any) {
console.error(`Failed to update Airtable for run ${task.runId}:`, error);
if (task.retries < MAX_RETRIES) {
airtableUpdateTasks[runId].retries += 1;
console.log(`Retrying task for runId: ${runId}, attempt: ${task.retries}`);
} else {
airtableUpdateTasks[runId].status = 'failed';
console.log(`Max retries reached for runId: ${runId}. Marking task as failed.`);
}
}
}
}
// Remove tasks after iteration
for (const runId of tasksToDelete) {
delete airtableUpdateTasks[runId];
}

Comment on lines +156 to +161
} catch (error: any) {
console.error(
"Error updating Airtable configuration:",
error.response?.data?.message || error.message
);
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Notify the user on Airtable configuration errors.

In the updateAirtableConfig function, errors are logged to the console but not communicated to the user. Informing the user about the error would improve the user experience.

Apply this diff:

         } catch (error: any) {
           console.error(
             "Error updating Airtable configuration:",
             error.response?.data?.message || error.message
           );
+          notify(
+            "error",
+            t('integration_settings.airtable.errors.update_error', {
+              message: error.response?.data?.message || error.message
+            })
+          );
         }

Ensure the translation key integration_settings.airtable.errors.update_error exists in your localization files.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} catch (error: any) {
console.error(
"Error updating Airtable configuration:",
error.response?.data?.message || error.message
);
}
} catch (error: any) {
console.error(
"Error updating Airtable configuration:",
error.response?.data?.message || error.message
);
notify(
"error",
t('integration_settings.airtable.errors.update_error', {
message: error.response?.data?.message || error.message
})
);
}

Comment on lines +181 to +198
// Add task for Google Sheets update
googleSheetUpdateTasks[id] = {
robotId: plainRun.robotMetaId,
runId: id,
status: 'pending',
retries: 5,
};
processGoogleSheetUpdates();

// Add task for Airtable update
airtableUpdateTasks[id] = {
robotId: plainRun.robotMetaId,
runId: id,
status: 'pending',
retries: 5,
};
processAirtableUpdates();

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling and consider refactoring update tasks.

While the implementation mirrors the Google Sheets structure, there are opportunities for improvement:

  1. Error handling is missing for both processGoogleSheetUpdates() and processAirtableUpdates() calls
  2. The task creation logic is duplicated and could be abstracted

Consider refactoring to:

+ const createIntegrationTask = (type: 'googleSheet' | 'airtable', id: string, robotId: string) => {
+   const tasks = type === 'googleSheet' ? googleSheetUpdateTasks : airtableUpdateTasks;
+   tasks[id] = {
+     robotId,
+     runId: id,
+     status: 'pending',
+     retries: 5,
+   };
+ };

+ try {
+   // Create and process Google Sheets task
+   createIntegrationTask('googleSheet', id, plainRun.robotMetaId);
+   await processGoogleSheetUpdates();
+
+   // Create and process Airtable task
+   createIntegrationTask('airtable', id, plainRun.robotMetaId);
+   await processAirtableUpdates();
+ } catch (error) {
+   logger.error(`Failed to process integration updates: ${error.message}`);
+   // Consider whether to fail the run or just log the error
+ }
-    // Add task for Google Sheets update
-    googleSheetUpdateTasks[id] = {
-      robotId: plainRun.robotMetaId,
-      runId: id,
-      status: 'pending',
-      retries: 5,
-    };
-    processGoogleSheetUpdates();
-
-    // Add task for Airtable update
-    airtableUpdateTasks[id] = {
-      robotId: plainRun.robotMetaId,
-      runId: id,
-      status: 'pending',
-      retries: 5,
-    };
-    processAirtableUpdates();

Committable suggestion skipped: line range outside the PR's diff.

@amhsirak amhsirak changed the title Airtable Integration feat: airtable integration Jan 23, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (1)
src/components/integration/IntegrationSettings.tsx (1)

381-394: Enhance input validation for Airtable configuration.

The current validation only checks for non-empty values. Consider adding more robust validation for the Base ID and Table Name fields.

Apply this diff:

   <TextField
     sx={{ marginTop: "15px" }}
     label={t('integration_settings.airtable.fields.base_id')}
     value={settings.airtableBaseId}
-    onChange={(e) => setSettings({ ...settings, airtableBaseId: e.target.value })}
+    onChange={(e) => {
+      const value = e.target.value.trim();
+      setSettings({ ...settings, airtableBaseId: value });
+    }}
+    error={settings.airtableBaseId.trim() !== '' && !/^app[a-zA-Z0-9]{14}$/.test(settings.airtableBaseId)}
+    helperText={settings.airtableBaseId.trim() !== '' && !/^app[a-zA-Z0-9]{14}$/.test(settings.airtableBaseId) 
+      ? t('integration_settings.airtable.errors.invalid_base_id')
+      : ''}
     fullWidth
   />
   <TextField
     sx={{ marginTop: "15px" }}
     label={t('integration_settings.airtable.fields.table_name')}
     value={settings.airtableTableName}
-    onChange={(e) => setSettings({ ...settings, airtableTableName: e.target.value })}
+    onChange={(e) => {
+      const value = e.target.value.trim();
+      setSettings({ ...settings, airtableTableName: value });
+    }}
+    error={settings.airtableTableName.trim() !== '' && settings.airtableTableName.length < 3}
+    helperText={settings.airtableTableName.trim() !== '' && settings.airtableTableName.length < 3
+      ? t('integration_settings.airtable.errors.invalid_table_name')
+      : ''}
     fullWidth
   />
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 722b97e and c9ffec9.

📒 Files selected for processing (2)
  • server/src/models/Robot.ts (4 hunks)
  • src/components/integration/IntegrationSettings.tsx (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • server/src/models/Robot.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/integration/IntegrationSettings.tsx

[error] 350-350: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (3)
src/components/integration/IntegrationSettings.tsx (3)

28-29: LGTM! Interface changes are well-documented.

The new Airtable-specific fields are properly typed and documented.


56-57: LGTM! State initialization is consistent.

The new Airtable fields are properly initialized with empty strings.


142-161: Notify the user on Airtable configuration errors.

The error handling needs improvement to notify users of configuration update failures.

{t('integration_settings.airtable.title')}
</Typography>

{recording && recording.airtable_base_id ? (
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use optional chaining for safer property access.

The code should use optional chaining to safely access the recording property.

Apply this diff:

-        {recording && recording.airtable_base_id ? (
+        {recording?.airtable_base_id ? (
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{recording && recording.airtable_base_id ? (
{recording?.airtable_base_id ? (
🧰 Tools
🪛 Biome (1.9.4)

[error] 350-350: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

Comment on lines +73 to +76
// Airtable Authentication
const authenticateWithAirtable = () => {
window.location.href = `${apiUrl}/auth/airtable?robotId=${recordingId}`;
};
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for missing recordingId.

The function should validate that recordingId exists before proceeding with authentication.

Apply this diff:

   const authenticateWithAirtable = () => {
+    if (!recordingId) {
+      notify("error", t('integration_settings.errors.missing_recording_id'));
+      return;
+    }
     window.location.href = `${apiUrl}/auth/airtable?robotId=${recordingId}`;
   };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Airtable Authentication
const authenticateWithAirtable = () => {
window.location.href = `${apiUrl}/auth/airtable?robotId=${recordingId}`;
};
// Airtable Authentication
const authenticateWithAirtable = () => {
if (!recordingId) {
notify("error", t('integration_settings.errors.missing_recording_id'));
return;
}
window.location.href = `${apiUrl}/auth/airtable?robotId=${recordingId}`;
};

Comment on lines +163 to +179
const removeIntegration = async (integrationType: 'google' | 'airtable') => {
try {
await axios.post(
`${apiUrl}/auth/gsheets/remove`,
`${apiUrl}/auth/${integrationType}/remove`,
{ robotId: recordingId },
{ withCredentials: true }
);

setRecording(null);
setSpreadsheets([]);
setSettings({ spreadsheetId: "", spreadsheetName: "", data: "" });
setSettings({
spreadsheetId: "",
spreadsheetName: "",
airtableBaseId: "",
airtableTableName: "",
data: "",
});
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling in removeIntegration function.

The function should notify users of errors and use more user-friendly error messages.

Apply this diff:

   } catch (error: any) {
+    const errorMessage = error.response?.data?.message || error.message;
     console.error(
       `Error removing ${integrationType} integration:`,
-      error.response?.data?.message || error.message
+      errorMessage
     );
+    notify(
+      "error",
+      t('integration_settings.errors.remove_integration', {
+        type: integrationType,
+        message: errorMessage
+      })
+    );
   }

Committable suggestion skipped: line range outside the PR's diff.

@AmitChauhan63390 AmitChauhan63390 deleted the integraton_airtable2 branch January 24, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features Type: Integration All issues/PRs related to integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants