[Improvement] Set polling URL, dynamic dropdowns and improve OAuth authorization #8901
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: https://app.greptile.com/review/github.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
14 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThe changes enhance the Zapier integration plugin by introducing a new method for notifying subscribers about timer status changes and improving the OAuth2 flow. A new event-driven architecture is established with updated module configurations and additional triggers for fetching dynamic data such as tenants and organizations. The input fields for timer operations are restructured for better usability, and error handling is improved across various components. Additionally, a service for managing temporary authentication codes is added, along with enhancements to logging and token management. Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
packages/plugins/integration-zapier/src/lib/zapier-webhook.service.ts(3 hunks)packages/plugins/integration-zapier/src/lib/zapier.module.ts(3 hunks)packages/plugins/integration-zapier/zapier/.gitignore(1 hunks)packages/plugins/integration-zapier/zapier/src/authentication.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts(4 hunks)packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts(4 hunks)packages/plugins/integration-zapier/zapier/src/index.ts(2 hunks)packages/plugins/integration-zapier/zapier/src/triggers/organizationContactList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/organizationList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/organizationTeamList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/projectList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/taskList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/tenantList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/timerStatus.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/plugins/integration-zapier/zapier/src/triggers/timerStatus.ts (1)
packages/core/src/lib/entity-subscription/entity-subscription.service.ts (1)
unsubscribe(76-100)
packages/plugins/integration-zapier/zapier/src/index.ts (1)
packages/plugins/integration-zapier/zapier/src/triggers/timerStatus.ts (1)
timerStatusKey(97-97)
🪛 GitHub Check: Cspell
packages/plugins/integration-zapier/zapier/src/triggers/timerStatus.ts
[warning] 54-54:
Unknown word (lastlog)
packages/plugins/integration-zapier/zapier/src/authentication.ts
[warning] 24-24:
Unknown word (acces)
🪛 GitHub Actions: Check Spelling and Typos with cspell
packages/plugins/integration-zapier/zapier/src/triggers/timerStatus.ts
[warning] 54-54: Unknown word (lastlog)
packages/plugins/integration-zapier/zapier/src/authentication.ts
[warning] 24-24: Unknown word (acces)
🪛 Biome (1.9.4)
packages/plugins/integration-zapier/zapier/src/authentication.ts
[error] 29-29: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/plugins/integration-zapier/zapier/src/triggers/organizationContactList.ts
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test
- GitHub Check: build
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (22)
packages/plugins/integration-zapier/zapier/.gitignore (1)
43-43: Good addition to ignore TypeScript build info files.Adding
*.tsbuildinfoto the gitignore is a good practice. These files are generated by the TypeScript compiler for incremental builds and should not be committed to version control.packages/plugins/integration-zapier/zapier/src/index.ts (2)
11-17: Well-structured imports for dynamic dropdown functionality.The addition of these hidden triggers for dynamic dropdowns is a good improvement to the Zapier integration, allowing for more interactive and user-friendly selection of related entities.
31-38: Good implementation of hidden triggers for dynamic dropdowns.These hidden triggers are properly added to the app definition, maintaining a consistent pattern with the existing triggers. The clear comment explaining their purpose enhances code readability.
packages/plugins/integration-zapier/zapier/src/triggers/organizationTeamList.ts (1)
27-52: Good configuration of the hidden trigger for dynamic dropdown.The configuration is well-structured with appropriate metadata, input fields, and sample data. The use of a dynamic reference to
organization_listfor the organization ID field is particularly useful.packages/plugins/integration-zapier/zapier/src/authentication.ts (2)
10-16: Good enhancement to the authentication test configuration.Adding the method and headers with the Bearer token improves the authentication test by properly validating the access token against the API.
19-40: Good addition of the connectionLabel method.The connectionLabel method enhances the user experience by providing a meaningful label for the connection in the Zapier UI, based on the user's information. The error handling ensures a fallback label is always provided.
🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: Cspell
[warning] 24-24:
Unknown word (acces)🪛 GitHub Actions: Check Spelling and Typos with cspell
[warning] 24-24: Unknown word (acces)
packages/plugins/integration-zapier/zapier/src/triggers/timerStatus.ts (2)
12-27: Improved response handling for both array and object formats.The new code properly handles both single object and array responses, ensuring consistent data structure for Zapier.
109-146: Updated sample data structure with detailed timer information.The sample data now accurately represents the timer status payload with comprehensive details like duration, running state, and detailed log information.
packages/plugins/integration-zapier/src/lib/zapier.module.ts (3)
3-3: Switched from command/query pattern to event-driven approach.The imports have been updated to include
EventsHandlerand localEventHandlers, indicating a shift to an event-driven architecture.Also applies to: 25-25
49-49: Changed from command/query handlers to event handlers.The module now uses event handlers instead of command and query handlers, aligning with the event-driven approach.
51-51:❓ Verification inconclusive
Removed TimerModule from exports.
TimerModule has been removed from the exports array, which could potentially break dependent code.
🏁 Script executed:
#!/bin/bash # Check if any files are importing TimerModule from ZapierModule echo "Checking for imports of TimerModule from ZapierModule..." rg --type ts "ZapierModule.*TimerModule|import.*TimerModule.*from.*zapier" --glob "!**/zapier.module.ts"Length of output: 229
Action Required: Verify TimerModule Removal Impact
The current search for TimerModule usage outside of the Zapier module did not yield any results, suggesting that no dependent code is directly importing TimerModule from ZapierModule. However, because the verification output was inconclusive (with no output produced), please manually verify that no indirect dependency exists that relies on TimerModule being re-exported from this module.
- File Affected:
packages/plugins/integration-zapier/src/lib/zapier.module.ts(Line 51)- Change Noted:
TimerModulewas removed from the exports array.- Next Steps: Manually inspect dependent code or run additional repository-wide searches for "TimerModule" usage to ensure its removal won’t inadvertently break any functionality.
packages/plugins/integration-zapier/src/lib/zapier-webhook.service.ts (1)
5-6: Added HttpService and RxJS for webhook notifications.The service now properly injects the HttpService and imports RxJS operators for error handling in the new notification method.
Also applies to: 15-15
packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts (6)
10-10: Good addition of Content-Type headerAdding the Content-Type header ensures the API server correctly processes the request body as JSON.
19-19: Proper boolean conversionConverting the string 'true' to a boolean value is necessary since Zapier typically passes form values as strings. This ensures the API receives the correct data type.
45-60: Well-structured dynamic dropdown fieldsThe tenant and organization fields now utilize dynamic dropdowns with helpful guidance text, which greatly improves the user experience.
89-93: Improved source field with Zapier defaultSetting the default source to 'ZAPIER' instead of 'BROWSER' makes more sense for this integration.
94-141: Enhanced input fields with dynamic dropdownsThe addition of descriptive fields with dynamic dropdowns for projects, tasks, contacts, and teams significantly improves the usability of the integration.
144-158: Updated sample data for better documentationThe sample data has been updated to reflect the actual response structure, which is helpful for users and developers.
packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts (4)
10-10: Good addition of Content-Type headerAdding the Content-Type header ensures the API server correctly processes the request body as JSON.
19-19: Proper boolean conversionConverting the string 'true' to a boolean value is necessary since Zapier typically passes form values as strings. This ensures the API receives the correct data type.
45-60: Well-structured dynamic dropdown fieldsThe tenant and organization fields now utilize dynamic dropdowns with helpful guidance text, which greatly improves the user experience.
93-140: Enhanced input fields with dynamic dropdownsThe addition of descriptive fields with dynamic dropdowns for projects, tasks, contacts, and teams significantly improves the usability of the integration.
packages/plugins/integration-zapier/zapier/src/triggers/organizationTeamList.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/authentication.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/authentication.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/authentication.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/triggers/organizationContactList.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/triggers/timerStatus.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-webhook.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts
Outdated
Show resolved
Hide resolved
|
View your CI Pipeline Execution ↗ for commit f955235.
☁️ Nx Cloud last updated this comment at |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
RolandM99
force-pushed
the
fix/zapier-plugin-integration
branch
from
c2cebf6 to
b43a04f
Compare
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR implements event handlers for timer events in the Zapier integration, enabling real-time notifications when timers are started or stopped.
- Added
ZapierTimerStartedHandlerandZapierTimerStoppedHandlerthat notify Zapier webhooks when timer status changes - Implemented dynamic dropdown triggers for entities (tenants, organizations, projects, tasks, contacts, teams) to improve UX in Zapier interface
- Added
notifyTimerStatusChangedmethod toZapierWebhookServiceto send webhook notifications with proper error handling - Added polling fallback in
timerStatus.tsfor when webhooks aren't available, using the/api/timesheet/timer/status/workedendpoint - Improved data handling in timer triggers to support both single objects and arrays, ensuring proper formatting for Zapier
Greptile AI: I've analyzed the changes to the Zapier integration plugin and focused on the most significant improvements that weren't covered in previous reviews.
25 file(s) reviewed, 23 comment(s)
Edit PR Review Bot Settings | Greptile
packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/triggers/organizationTeamList.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/triggers/organizationContactList.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/triggers/organizationList.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/triggers/taskList.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 18
🔭 Outside diff range comments (1)
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (1)
93-152: 🛠️ Refactor suggestionPotential mismatch between newly generated auth code and redirect parameters.
Inside
callback, a new authorization code is generated (line 128), but the redirect query is built usingquery.codeinstead of the freshly mintedcode. This could prevent the intended code from being passed back to Zapier.Consider updating the redirect logic to include the new code:
- const queryParamsString = buildQueryString({ - code: query.code, - state: query.state - }); + const queryParamsString = buildQueryString({ + code, + state: query.state + });
♻️ Duplicate comments (4)
packages/plugins/integration-zapier/zapier/src/triggers/organizationList.ts (1)
16-31: Add validation for API response structure.The code already includes validation for the API response structure, which is a good practice for ensuring robustness when dealing with external APIs.
packages/plugins/integration-zapier/zapier/src/triggers/taskList.ts (1)
3-30: 🧹 Nitpick (assertive)Strengthen data validation before mapping tasks
The function properly validates project ID and has good error handling, but should check if response.data and response.data.items exist before mapping.
const perform = async (z: ZObject, bundle: Bundle) => { const projectId = bundle.inputData.projectId; if(!projectId) { throw new Error('Project ID is required') } try { const response = await z.request({ url: `${process.env.API_BASE_URL}/api/tasks`, method: 'GET', headers: { Authorization: `Bearer ${bundle.authData['access_token']}`, }, params: { projectId: projectId } }); + if (!response.data || !Array.isArray(response.data.items)) { + throw new Error('Unexpected API response format'); + } return response.data.items.map((task: { id: string; title?: string; name?: string }) => ({ id: task.id, name: task.title || task.name })); } catch (error) { z.console.error('Error fetching tasks:', error); - throw new Error('Failed to fetch tasks'); + throw new Error(`Failed to fetch tasks: ${error.message || 'Unknown error'}`); } };packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts (2)
81-91: Inconsistency in source field choices and defaultThe source field has inconsistent configuration:
- Default value is set to 'BROWSER' (line 90)
- 'ZAPIER' option is missing from the choices list
From the context and other changes, it appears 'ZAPIER' should be an available option and possibly the default.
choices: { BROWSER: 'Browser', DESKTOP: 'Desktop', MOBILE: 'Mobile', BROWSER_EXTENSION: 'Browser Extension', HUBSTAFF: 'Hubstaff', UPWORK: 'Upwork', TEAMS: 'Teams', - CLOC: 'Cloc' + CLOC: 'Cloc', + ZAPIER: 'Zapier' }, - default: 'BROWSER', + default: 'ZAPIER', helpText: 'The source of this timer entry'
149-149: Inconsistency in sample dataThe sample data shows source as 'BROWSER' but should be consistent with the default setting for the field.
- source: 'BROWSER', + source: 'ZAPIER',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
packages/core/src/lib/auth/auth.controller.ts(2 hunks)packages/core/src/lib/core/context/request-context.ts(1 hunks)packages/plugins/integration-zapier/src/lib/handlers/index.ts(1 hunks)packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-started.handler.ts(1 hunks)packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-stopped.handler.ts(1 hunks)packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts(2 hunks)packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts(5 hunks)packages/plugins/integration-zapier/src/lib/zapier-webhook.service.ts(2 hunks)packages/plugins/integration-zapier/src/lib/zapier.config.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier.controller.ts(2 hunks)packages/plugins/integration-zapier/src/lib/zapier.module.ts(2 hunks)packages/plugins/integration-zapier/src/lib/zapier.service.ts(5 hunks)packages/plugins/integration-zapier/zapier/src/authentication.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts(3 hunks)packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts(4 hunks)packages/plugins/integration-zapier/zapier/src/triggers/organizationContactList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/organizationList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/organizationTeamList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/projectList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/taskList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/tenantList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/timerStatus.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/plugins/integration-zapier/src/lib/zapier.controller.ts (1)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8863
File: packages/plugins/integration-zapier/src/lib/zapier.controller.ts:14-17
Timestamp: 2025-04-09T19:08:05.034Z
Learning: In the Ever Gauzy codebase, endpoints related to Zapier integration (and likely other integrations) are intentionally restricted to users with INTEGRATION_ADD and INTEGRATION_EDIT permissions, even for read-only operations. This is by design to ensure only users with integration management privileges can access integration data.
🧬 Code Graph Analysis (6)
packages/plugins/integration-zapier/zapier/src/triggers/timerStatus.ts (1)
packages/core/src/lib/entity-subscription/entity-subscription.service.ts (1)
unsubscribe(76-100)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (2)
packages/plugins/integration-zapier/src/lib/zapier-webhook.service.ts (1)
Injectable(8-118)packages/desktop-core/src/lib/logger/logger.ts (1)
Logger(5-32)
packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts (1)
packages/desktop-core/src/lib/logger/logger.ts (1)
Logger(5-32)
packages/plugins/integration-zapier/src/lib/zapier.module.ts (2)
packages/plugins/integration-zapier/src/lib/repository/mikro-orm-zapier.repository.ts (1)
MikroOrmZapierWebhookSubscriptionRepository(4-4)packages/plugins/integration-zapier/src/lib/handlers/index.ts (1)
EventHandlers(5-5)
packages/core/src/lib/core/context/request-context.ts (1)
packages/contracts/src/lib/user.model.ts (1)
IUser(40-82)
packages/plugins/integration-zapier/src/lib/zapier.service.ts (3)
packages/contracts/src/lib/zapier.model.ts (2)
IZapierAccessTokens(3-8)ICreateZapierIntegrationInput(12-18)packages/contracts/src/lib/integration.model.ts (1)
IIntegrationEntitySetting(20-26)packages/plugins/integration-zapier/src/lib/zapier.config.ts (1)
ZAPIER_TOKEN_EXPIRATION_TIME(17-17)
🪛 Biome (1.9.4)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
[error] 28-28: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
packages/plugins/integration-zapier/src/lib/zapier.service.ts
[error] 82-82: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (62)
packages/plugins/integration-zapier/zapier/src/triggers/organizationTeamList.ts (1)
1-53: Good implementation of the organization team list trigger.The trigger is well-structured with proper error handling, input validation, and appropriate return of response data. The dynamic dropdown configuration is correctly set up with the organization list reference.
packages/plugins/integration-zapier/src/lib/handlers/index.ts (1)
1-5: LGTM. Clean export of event handlers.The file correctly imports and exports the Zapier timer event handlers in a well-structured array, following good module organization practices.
packages/plugins/integration-zapier/src/lib/zapier.config.ts (2)
7-9: LGTM. Well-defined callback URL constant.The
ZAPIER_CALLBACK_URLis properly defined and documented with an appropriate JSDoc comment, using the environment variable for the API base URL.
16-17: LGTM. Appropriate token expiration time constant.The
ZAPIER_TOKEN_EXPIRATION_TIMEis set to a reasonable value (1 hour) and documented clearly.packages/core/src/lib/core/context/request-context.ts (1)
219-249:❓ Verification inconclusive
Ensure
organizationsarray contains the expected structure.The method provides a good fallback mechanism to determine the current organization ID. However, when accessing
user.organizations[0].organizationId, verify that theIUserOrganizationinterface has anorganizationIdproperty.
🏁 Script executed:
#!/bin/bash # Check the structure of the IUserOrganization interface rg -A 15 "interface IUserOrganization" --type typescriptLength of output: 93
Attention: Verify the IUserOrganization Interface Structure
The current implementation of
RequestContext.currentOrganizationIdcorrectly attempts to determine the current organization ID by checking several user properties. However, since the fallback logic relies on accessinguser.organizations[0].organizationId, please ensure that theIUserOrganizationinterface indeed includes theorganizationIdproperty.To verify this, note that the initial attempt to search for the interface using a file type filter (
--type typescript) resulted in an error. Please run the following command (without file type restrictions) to check the definition:Manually verify that the output shows an
organizationIdproperty within theIUserOrganizationinterface. Once confirmed, no further changes to the fallback logic in this method are needed.
🏁 Script executed:
#!/bin/bash rg -A 15 "interface IUserOrganization" .Length of output: 1021
Action Required: Verify the Presence of
organizationIdin the User Organization InterfacesThe fallback logic in
currentOrganizationId()appears sound. However, since theIUserOrganizationinterface extendsIBaseUserOrganizationwithout explicitly specifying any properties, please confirm that theorganizationIdproperty is defined—either directly onIUserOrganizationor within its base interface (IBaseUserOrganization). The grep results indicate that the property isn’t directly declared inIUserOrganization, so you'll need to verify the structure ofIBaseUserOrganization.
- Steps:
- Manually inspect or run an additional search (e.g., using
rg -A 20 "interface IBaseUserOrganization" .) to locate the definition oforganizationIdwithinIBaseUserOrganization.- If the property is missing, update the interfaces accordingly to ensure that the fallback in
currentOrganizationId()works as expected.packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts (3)
83-83: Good addition of a class-level logger instance.Using a dedicated logger instance with the class name improves consistency and traceability of log messages.
91-99: Good implementation of OAuth credentials validation.The added code properly checks for Zapier OAuth credentials and provides appropriate feedback. This will help administrators quickly identify configuration issues during setup.
108-108: Good consistency improvement in logging.Changing from console.log to the class logger instance improves consistency in logging and follows better practices for production code.
packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-started.handler.ts (4)
1-5: Good implementation of imports and dependencies.The imports are properly organized and include all necessary dependencies from NestJS CQRS, Gauzy core, and local services.
6-9: Correctly implemented class declaration with appropriate decorators.The class correctly implements the
IEventHandlerinterface and is decorated with both@Injectable()and@EventsHandler(TimerStartedEvent)to properly register it in the NestJS dependency injection system and event handling system.
10-15: Well-documented method with JSDoc.The JSDoc comments clearly explain the purpose and parameters of the handler method, which is good practice for maintainability.
16-22: Correctly implemented event handler logic.The handler properly processes the TimerStartedEvent and forwards the relevant data to the webhook service. The structure follows good practices for event handling in NestJS.
packages/plugins/integration-zapier/zapier/src/authentication.ts (3)
4-4: Proper import of Zapier types.The import of ZObject and Bundle types from zapier-platform-core ensures proper type checking, improving code quality and preventing potential errors.
10-16: Enhanced authentication test configuration.The improved test configuration properly defines the method and headers with bearer token authentication, which is essential for validating OAuth2 tokens correctly.
18-41: Well-implemented connection label method with proper error handling.The connectionLabel method effectively fetches user data and provides appropriate fallbacks. I see you've addressed previous review comments by:
- Using optional chaining for safer property access
- Ensuring consistent naming in the fallback connection label
- Correctly using the access_token property name
packages/plugins/integration-zapier/zapier/src/triggers/organizationContactList.ts (5)
3-7: Well-defined Contact interface.The Contact interface properly defines the expected structure of contact objects with required properties and an index signature for additional properties, improving type safety.
9-15: Good input validation.The function properly validates the required organizationId parameter and throws a descriptive error if it's missing, which is essential for providing a good user experience.
16-30: Proper API request with response validation.The function makes an API request with correct authorization headers and validates the API response format, addressing a previous review comment. This ensures more robust error handling if the API changes.
31-38: Effective error handling and data transformation.The function properly transforms the API response to a simpler structure and includes comprehensive error handling with informative error messages and console logging for debugging.
41-65: Well-structured trigger definition.The trigger definition follows Zapier best practices with:
- Clear identification (key, noun)
- User-friendly display properties
- Properly configured input fields with dynamic dropdown
- Sample data for testing
The hidden property ensures it's only used for dynamic dropdowns and not exposed directly to users.
packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-stopped.handler.ts (1)
1-24: Good implementation of the event handler!The handler follows NestJS best practices with proper dependency injection and event handling. The code is well-documented with clear JSDoc comments.
For future enhancement, consider adding explicit error handling within the
handlemethod to gracefully manage notification failures, especially if notifications are critical to your application flow.packages/plugins/integration-zapier/zapier/src/triggers/projectList.ts (2)
9-31: Well-structured API request with proper validation.The implementation correctly validates required inputs and API response structure, following best practices for Zapier triggers.
42-67: Good trigger configuration with clear documentation.The trigger configuration is well structured with appropriate metadata, labels, and help text. The dynamic dropdown for organization selection provides a good user experience.
packages/plugins/integration-zapier/zapier/src/triggers/tenantList.ts (2)
3-7: LGTM! Well-defined Tenant interfaceThe Tenant interface properly defines the structure expected from the API response with appropriate types.
33-48: LGTM! Well-structured export with clear metadataThe export definition provides all necessary Zapier-specific metadata and correctly sets
hidden: truefor this dynamic dropdown component.packages/plugins/integration-zapier/zapier/src/triggers/taskList.ts (1)
32-57: LGTM! Well-structured trigger with dynamic field dependenciesThe trigger definition is well-structured with clear labels and appropriate field configurations. The dynamic dropdown dependency for projects is correctly implemented.
packages/plugins/integration-zapier/zapier/src/triggers/timerStatus.ts (2)
109-109: LGTM! performList added to operationsGood addition of the performList function to the operations object, providing a polling fallback when webhooks aren't available.
113-146: LGTM! Comprehensive sample data structureThe updated sample data structure provides a detailed representation of timer status including the lastLog object, which helps Zapier users understand the data format.
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (2)
10-18: LGTM! Well-structured in-memory storage for auth codesThe in-memory Map implementation with clear type definitions is appropriate for temporary auth codes.
51-69: LGTM! Secure implementation of code validationGood implementation of the code validation logic with immediate deletion after use to prevent reuse attacks.
packages/plugins/integration-zapier/src/lib/zapier.controller.ts (2)
16-16: Import looks safe.
No concerns with addingIZapierAccessTokenshere, assuming it may be used in future additions.
80-101: Robust error handling approach.
Re-throwing known exceptions and defaulting to a 500-level error for unknown cases is clear and consistent.packages/plugins/integration-zapier/src/lib/zapier-webhook.service.ts (2)
5-6: New imports for HTTP and RxJS operators look correct.
These additions are necessary for making HTTP requests and handling stream operations.
14-14: Injection of HttpService is appropriate.
Leveraging the service for webhook notifications aligns with NestJS best practices.packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts (10)
10-10: JSON content type is correct.
Ensures proper serialization and deserialization for the request payload.
41-41: Updated description is concise.
A simple and clear explanation of what the create does.
45-60: Dynamic fields for tenant and organization.
Adding these improves user guidance and ensures correct data selection.
90-92: Help text forsourceprovides clarity.
It’s beneficial to explain how the source field is used.
93-99: Description field adds flexibility.
Allows users to specify details about the tracked work.
109-140: Project, task, contact, and team fields bolster customization.
Dynamically populated options improve user experience.
144-146: Sample data now aligns with the new structure.
Demonstrates realistic usage for tenant and organization fields.
150-150: Providing a descriptive message fordescription.
Makes the sample usage clearer.
153-153: UpdatedprojectIdreflects the new field.
Good consistency with the prior input definitions.
156-156: AddedorganizationTeamIdin sample data.
Ensures users see how to include team information.packages/plugins/integration-zapier/src/lib/zapier.module.ts (2)
25-26: Good introduction of new event-driven and auth code services.Replacing command/query handlers with
EventHandlersaligns with the shift toward an event-driven architecture, and importingZapierAuthCodeServicenicely complements that approach.
45-53: Providers and exports alignment looks solid.
ZapierAuthCodeServiceandEventHandlersare properly registered. Exporting only the needed services helps maintain a clean public API.packages/plugins/integration-zapier/src/lib/zapier.service.ts (5)
1-5: New imports for exceptions and UUID generation are appropriate.Using built-in NestJS exceptions and UUIDs is a solid approach for robust error handling and token generation.
17-17: Typed return for IZapierAccessTokens enhances clarity.Specifying
IZapierAccessTokensfor the token-related methods helps ensure consistent handling of token data.
26-26: Importing config constants is consistent with the rest of the module.Using
ZAPIER_AUTHORIZATION_URLandZAPIER_TOKEN_EXPIRATION_TIMEfromzapier.configcentralizes configuration logic.
30-30: Using a dedicated logger improves observability.Capturing errors and key events via
Loggeraids in debugging and maintenance.
146-238: NewcreateIntegrationmethod is clear and well-structured.Creating a fresh integration with tokens, populating default entity settings, and updating or creating the
IntegrationTenantare handled thoroughly. The approach to unify creation and token generation fosters maintainability.packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (3)
42-76: Comprehensive checks inauthorizemethod.
- Validating
zapier_redirect_uri,state,response_type, andclient_idensures a secure OAuth flow.- Redirecting to the login page with relevant parameters is a sensible approach.
153-217:getTokenmethod properly validates and creates integration tokens.
- Validating grant type, client credentials, and the authorization code is thorough.
- The use of
this.zapierService.createIntegrationstandardizes token creation across the module.
219-278:refreshTokenendpoint is aligned with service logic.
- Properly checks grant type and client credentials.
- Delegates to
zapierService.refreshTokenfor consistent token refreshing.packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts (8)
10-10: Good addition of the Content-Type headerAdding the explicit 'Content-Type' header ensures proper handling of the JSON request body by the API.
19-19: Improved boolean conversion for isBillableThe strict equality check
=== 'true'is a good improvement for explicit boolean conversion.
41-41: Clear description for the Zapier actionThe updated description clearly explains what this action does in Gauzy.
45-60: Good implementation of dynamic dropdown for tenant and organizationThe dynamic dropdowns for tenant and organization IDs with clear help text improve user experience significantly.
93-99: Good addition of description fieldAdding a description field with appropriate help text improves the usability of the timer functionality.
100-107: Improved isBillable field configurationThe billable field has been properly configured with the correct type (boolean) and helpful description.
109-140: Excellent implementation of dynamic dropdowns for related entitiesThe implementation of dynamic dropdowns for project, task, contact, and team entities greatly improves the user experience by allowing selection from existing records rather than requiring manual ID entry.
144-146: Sample data properly updated with realistic valuesThe sample response has been properly updated with realistic UUID values and additional fields.
Also applies to: 150-156
packages/plugins/integration-zapier/zapier/src/triggers/organizationList.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/triggers/organizationList.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (4)
packages/core/src/lib/auth/auth.controller.ts (1)
169-169: 🧹 Nitpick (assertive)Consider using a dedicated DTO for improved type safety.
While the inline type
{ zapier_state?: string }is better thanany, using a dedicated DTO would further improve type safety and enable validation with class-validator decorators.+// Add this to your DTOs +export class ZapierOAuthQueryDTO { + @IsString() + @IsOptional() + zapier_redirect_uri?: string; + + @IsString() + @IsOptional() + zapier_state?: string; +} -async loginSuccess(@Query() query: { zapier_state?: string }, @Res() res: Response) { +async loginSuccess(@Query() query: ZapierOAuthQueryDTO, @Res() res: Response) {packages/plugins/integration-zapier/zapier/src/triggers/taskList.ts (1)
29-32: 🛠️ Refactor suggestionInclude original error details in the error message.
When catching errors, you log the original error but throw a generic error message. Including the original error details would make troubleshooting easier.
} catch (error) { z.console.error('Error fetching tasks:', error); - throw new Error('Failed to fetch tasks'); + throw new Error(`Failed to fetch tasks: ${error.message || 'Unknown error'}`); }packages/plugins/integration-zapier/zapier/src/triggers/organizationList.ts (1)
17-17:⚠️ Potential issueFix the fallback URL logic.
The current fallback URL implementation won't work as expected. String interpolation will always produce a string (even empty), which is truthy in JavaScript/TypeScript.
- const baseUrl = `${process.env.API_BASE_URL}` || 'http://localhost:3000'; + const baseUrl = process.env.API_BASE_URL || 'http://localhost:3000';packages/plugins/integration-zapier/src/lib/zapier.module.ts (1)
14-15: Re-check ifTimerModuleis needed.Since there's a past comment about removing
TimerModuleif it's unused, please verify whether it's actually utilized or exported. If it's no longer necessary, removing it will reduce complexity.Run this script to see if
TimerModuleis referenced anywhere else:#!/bin/bash fd -e ts 'TimerModule' --exec rg 'TimerModule' {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
packages/core/src/lib/auth/auth.controller.ts(2 hunks)packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts(2 hunks)packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts(5 hunks)packages/plugins/integration-zapier/src/lib/zapier-webhook.service.ts(2 hunks)packages/plugins/integration-zapier/src/lib/zapier.controller.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier.module.ts(2 hunks)packages/plugins/integration-zapier/src/lib/zapier.service.ts(5 hunks)packages/plugins/integration-zapier/zapier/src/authentication.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts(4 hunks)packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts(4 hunks)packages/plugins/integration-zapier/zapier/src/triggers/organizationContactList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/organizationList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/organizationTeamList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/projectList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/taskList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/tenantList.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/triggers/timerStatus.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts (1)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts:100-107
Timestamp: 2025-04-10T15:31:05.841Z
Learning: In Zapier integrations, boolean configuration fields need to use type 'boolean' but with string default values like 'true' or 'false' because that's what the Zapier platform expects to receive. The actual boolean conversion is handled in the code with a comparison like `inputData['fieldName'] === 'true'`.
🧬 Code Graph Analysis (8)
packages/plugins/integration-zapier/zapier/src/triggers/timerStatus.ts (1)
packages/core/src/lib/entity-subscription/entity-subscription.service.ts (1)
unsubscribe(76-100)
packages/core/src/lib/auth/auth.controller.ts (1)
packages/common/src/lib/decorators/public.decorator.ts (1)
Public(15-15)
packages/plugins/integration-zapier/src/lib/zapier-webhook.service.ts (3)
packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-started.handler.ts (1)
Injectable(6-23)packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-stopped.handler.ts (1)
Injectable(6-24)packages/plugins/integration-zapier/src/lib/repository/type-orm-zapier.repository.ts (1)
Injectable(6-13)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (2)
packages/plugins/integration-zapier/src/lib/zapier-webhook.service.ts (1)
Injectable(8-118)packages/desktop-core/src/lib/logger/logger.ts (1)
Logger(5-32)
packages/plugins/integration-zapier/src/lib/zapier.module.ts (2)
packages/plugins/integration-zapier/src/lib/repository/mikro-orm-zapier.repository.ts (1)
MikroOrmZapierWebhookSubscriptionRepository(4-4)packages/plugins/integration-zapier/src/lib/handlers/index.ts (1)
EventHandlers(5-5)
packages/plugins/integration-zapier/src/lib/zapier.controller.ts (2)
packages/core/src/lib/shared/decorators/permissions.decorator.ts (1)
Permissions(5-5)packages/contracts/src/lib/zapier.model.ts (1)
IZapierEndpoint(20-24)
packages/plugins/integration-zapier/src/lib/zapier.service.ts (3)
packages/contracts/src/lib/zapier.model.ts (2)
IZapierAccessTokens(3-8)ICreateZapierIntegrationInput(12-18)packages/contracts/src/lib/integration.model.ts (1)
IIntegrationEntitySetting(20-26)packages/plugins/integration-zapier/src/lib/zapier.config.ts (1)
ZAPIER_TOKEN_EXPIRATION_TIME(17-17)
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (3)
packages/plugins/integration-zapier/src/lib/dto/create-zapier-integration.dto.ts (1)
CreateZapierIntegrationDto(6-31)packages/contracts/src/lib/zapier.model.ts (2)
IZapierAccessTokens(3-8)ZapierGrantType(10-10)packages/plugins/integration-zapier/src/lib/zapier.config.ts (1)
ZAPIER_TOKEN_EXPIRATION_TIME(17-17)
🪛 Biome (1.9.4)
packages/plugins/integration-zapier/zapier/src/triggers/taskList.ts
[error] 22-22: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts
[error] 31-31: This code is unreachable
... because either this statement will return from the function ...
... or this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (63)
packages/core/src/lib/auth/auth.controller.ts (2)
11-14: LGTM! Necessary imports added for new functionality.The added imports are appropriate for the new Zapier OAuth flow implementation:
BadRequestExceptionfor error handling,Resfor response manipulation, and theResponsetype from Express for proper typing.
154-187: Well implemented Zapier OAuth success handler with proper security measures.The implementation properly handles the Zapier OAuth flow with appropriate error handling, security considerations, and fallback paths. The code successfully addresses previous review comments by:
- Including the
@Public()decorator to make the endpoint accessible without authentication- Adding proper validation for
API_BASE_URLwith a fallback value- Sanitizing the
zapier_stateparameter withencodeURIComponent- Implementing error logging before redirecting
The method appropriately differentiates between Zapier OAuth flow and regular login flow based on the presence of the
zapier_stateparameter.packages/plugins/integration-zapier/zapier/src/authentication.ts (2)
10-16: Well-structured authentication test configuration.The test configuration is now properly structured with clear method, URL, and authorization headers. This improves the authentication testing process.
19-40: Good implementation of the connectionLabel method.The connectionLabel method provides a user-friendly way to identify connections in the Zapier UI. The implementation correctly:
- Fetches user data from the API
- Creates a label based on user name or email with fallback
- Uses proper error handling with try/catch
- Handles unexpected data structures with optional chaining
This is a significant improvement to the user experience.
packages/plugins/integration-zapier/zapier/src/triggers/organizationContactList.ts (3)
1-7: Good use of TypeScript interface.The Contact interface provides clear typing for the contact objects, which improves code readability and type safety.
16-38: Well-implemented error handling and response validation.The perform function includes robust error handling with:
- Proper API request with authentication
- Response structure validation
- Error logging and meaningful error messages
- Try/catch block to prevent unhandled exceptions
This implementation ensures reliable behavior even when the API response is unexpected.
41-66: Well-configured dynamic dropdown trigger.The trigger is properly configured as a hidden dynamic dropdown with:
- Clear labels and descriptions
- Required field validation
- Dynamic reference to organization list
- Sample data for testing
This implementation follows Zapier's best practices for dynamic dropdowns.
packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts (3)
77-77: Good use of class-level logger.Adding a class-level logger instance improves consistency and follows best practices for NestJS applications.
85-93: Improved OAuth credentials validation.The added code properly validates the presence of Zapier OAuth credentials and provides clear warning messages when they're missing, which helps with troubleshooting.
102-102: Consistent logging approach.Using the class logger for plugin destruction logging is more consistent with NestJS practices than using console.log directly.
packages/plugins/integration-zapier/zapier/src/triggers/organizationTeamList.ts (2)
1-7: Good use of TypeScript interface.The OrganizationTeam interface provides clear typing for the team objects, improving code readability and type safety.
37-62: Well-configured dynamic dropdown trigger.The trigger is properly configured as a hidden dynamic dropdown with:
- Clear labels and descriptions
- Required field validation
- Dynamic reference to organization list
- Sample data for testing
This implementation follows Zapier's best practices for dynamic dropdowns.
packages/plugins/integration-zapier/zapier/src/triggers/taskList.ts (1)
40-41: Description clearly identifies dynamic dropdown purpose.The description now clearly indicates that this trigger is for dynamic dropdown selection, which helps with developer understanding.
packages/plugins/integration-zapier/zapier/src/triggers/tenantList.ts (3)
19-26: Good error handling with comprehensive validation.Your implementation includes thorough response validation:
- Checking if the response exists
- Validating the expected data structure
This prevents potential runtime errors and provides clear error messages.
32-35: Well implemented error handling with detailed error messages.The error handling includes the original error message which is excellent for troubleshooting issues.
38-53: Well structured trigger configuration.The trigger configuration is complete with appropriate metadata, clear descriptions, and a representative sample. The hidden property correctly indicates this is for dynamic dropdowns.
packages/plugins/integration-zapier/zapier/src/triggers/organizationList.ts (3)
24-25: Good use of object shorthand property.Using the shorthand syntax for the
paramsobject makes the code more concise and follows modern JavaScript best practices.
27-30: Good response validation implementation.Validating both the existence and structure of the response data prevents potential runtime errors and provides a clear error message for troubleshooting.
36-38: Good error handling with detailed error message.Including the original error message in the thrown error makes troubleshooting much easier.
packages/plugins/integration-zapier/zapier/src/triggers/projectList.ts (2)
28-30: Good response validation implementation.Validating both the existence and structure of the response data prevents potential runtime errors and provides a clear error message.
36-39: Good error handling with detailed error message.Including the original error message in the thrown error makes troubleshooting much easier.
packages/plugins/integration-zapier/zapier/src/triggers/timerStatus.ts (3)
24-27: Looks good.
Mapping the array items is straightforward and ensures each item has an ID. The spread operator usage is clear and succinct.
34-37: Clear and concise docstring.
The docstring for the fallback polling action accurately describes its purpose.
39-66: Well-structured fallback polling function.
TheperformListfunction cleanly handles potential errors and validates the response format before mapping the data. The error handling via thetry/catchblock is also consistent with the rest of the codebase.packages/plugins/integration-zapier/src/lib/zapier-webhook.service.ts (2)
5-6: Good use of RxJS imports.
ImportingcatchError,firstValueFrom, andofensures modern RxJS practices without deprecated methods liketoPromise().
14-14: HTTP service injection aligned with NestJS best practices.
This underscores proper dependency injection for making external requests in a testable manner.packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts (6)
10-10: Explicit content type is beneficial.
Passing'Content-Type': 'application/json'ensures the server correctly interprets the request body.
19-19: Correct string-to-boolean conversion.
Usingbundle.inputData['isBillable'] === 'true'is consistent with the known Zapier platform requirement for boolean fields in string format. This is aligned with your recorded learnings.
28-31: Error handling block is solid.
Logging the error within thecatchblock and rethrowing a new error with context is a clean pattern. The static analysis tool’s “unreachable code” flag for line 31 is a false positive—this simply closes the function.🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: This code is unreachable
... because either this statement will return from the function ...
... or this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
41-41: Updated description is precise and user-friendly.
Changing the text to “Stops a timer in Gauzy.” clarifies the action for users.
45-60: Comprehensive input fields for stopping timers.
- Dynamically populating tenant, organization, project, task, contact, and team IDs enhances usability.
- Providing help text for each field is excellent for guiding Zapier users.
- Using
'false'as the default string for the booleanisBillablefield is the correct approach given Zapier’s constraints, backed by your learnings.Also applies to: 93-99, 100-107, 109-116, 117-124, 125-132, 133-140
144-156: Sample data accurately reflects new fields.
Including relevant IDs and an illustrative description helps users test their Zap configurations. The expanded sample is consistent with the new input fields.packages/plugins/integration-zapier/src/lib/zapier.module.ts (4)
25-25: Confirm correct usage ofEventHandlers.Including
EventHandlerssuggests a shift to an event-driven pattern. Verify that each handler inEventHandlersis thoroughly tested to ensure no unhandled event logic.
26-26: Well-structured service import.Importing
ZapierAuthCodeServicehere is appropriate. Ensure it’s only provided in one module to avoid potential multiple instantiations or confusion about its lifecycle.
48-48: Provider addition looks good.Registering
ZapierAuthCodeServicein providers allows injection across the module. Be sure to implement tests for methods that rely on this service.
53-53: Confirm exports alignment.Exporting
ZapierAuthCodeServicealongsideZapierServiceandZapierWebhookServiceis helpful for consumers outside this module. Make sure there are no unintentional exports and that other modules import only what they need.packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (2)
29-53: Validate concurrency usage forMap.The
generateAuthCodemethod internally mutates a sharedMap. If multiple requests come in simultaneously, consider concurrency-safe operations or external locking mechanisms to avoid potential race conditions in high-throughput scenarios.
61-84: Good practice of one-time code usage.In
getUserInfoFromAuthCode, you delete the code after a successful check, preventing replay attacks. This is a sound security choice.packages/plugins/integration-zapier/src/lib/zapier.service.ts (3)
1-30: Imports and logger setup are fine.Bringing in
Logger,uuidv4, and typed interfaces clarifies your intent. The new logger property ensures consistent debugging messages.
62-120: Refined token refresh logic.
- Error handling: Appropriately raises
NotFoundExceptionandBadRequestExceptionfor missing settings.- Performance: The for-loop to collect
client_id,client_secret, andrefresh_tokenis more performant than using a.reduce()with spreads.- Security: Generating new tokens with
uuidv4()is straightforward, but confirm if any cryptographic randomness or hashing is desired.Overall, these changes look robust.
142-234: Robust creation of Zapier integration.
- Integration tenant: Using
IntegrationTenantUpdateOrCreateCommandfor consistent domain logic is a good approach.- Dynamic entity settings: Mapping relevant organization and tenant IDs ensures each tied entity belongs to the correct scope.
- Export testing: Ensure a thorough test includes verifying the final stored settings (e.g.,
client_id,client_secret,access_token, etc.).No critical concerns; these improvements nicely enhance the Zapier integration flow.
packages/plugins/integration-zapier/src/lib/zapier.controller.ts (6)
13-14: Imports align with usage
These imports effectively introduce thePermissionsEnumand theTenantPermissionGuardand others used below. No issues found.
24-24: Constructor injection looks fine
ThezapierServiceis injected and used, and there are no obvious issues here.
26-34: Consistent API documentation
Declaring a description of "Successfully retrieved Zapier triggers" and specifying 400 for a missing/empty token is consistent with the enforced validations invalidateToken.
35-43: Trigger fetching logic
The new/triggersendpoint validates the token and fetches triggers fromzapierService.fetchTriggers(token). The error handling is appropriate.
45-63: Actions endpoint approach
The/actionsendpoint mirrors/triggers, but usesvalidateToken(token, true), causing a different exception (UnauthorizedException) for missing tokens. Confirm this is intentional if the design expects more stringent requirements for actions.Would you like to verify the expected behavior for missing tokens on both endpoints, ensuring the difference in thrown exceptions is truly intended?
67-68: Token validation helper
ThevalidateToken()method throwsBadRequestExceptionorUnauthorizedExceptionbased on the boolean flag. Naming and usage look consistent, and no issues are apparent.packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (6)
1-12: Imported modules reflect updated OAuth flow
All imported entities (exceptions, DTOs, config, services) appear to be used in the new authorization logic and token management code below.
15-18: Controller class setup
Tagging and logger initialization are standard and pose no issues.
20-24: Dependency injection
All injected dependencies (ConfigService,ZapierAuthCodeService,ZapierService) align with the methods in this controller.
86-145: callback() method overwrites the code
A new authorization code is generated (line 128), yet the final redirect sendsquery.code(line 136). Ensure this is intentional and that Zapier expects or uses the original code parameter rather than the newly generated one.Would you like to confirm that sending the original
query.codeback to Zapier is correct, or should we forward the newly generated code?
153-217: Token endpoint handles one-time codes
This approach properly validates credentials, retrieves user info tied to the code, and constructs an integration. Consider confirming that the code is invalidated or marked used to prevent reuse in undesired scenarios.
42-75:❓ Verification inconclusive
authorize() method environmental assumptions
The method correctly checks required parameters (redirectUri,state,responseType,clientId). Make sureprocess.env['CLIENT_BASE_URL']is consistently defined in each environment, or fallback gracefully to avoid broken redirects.
🏁 Script executed:
#!/bin/bash # Verify that CLIENT_BASE_URL is set in environment variables env | grep CLIENT_BASE_URLLength of output: 28
Action Required: Confirm CLIENT_BASE_URL Configuration
Theauthorize()method correctly validates the required parameters. However, the verification command did not output a definedCLIENT_BASE_URLin the current environment. Please ensure that this environment variable is set consistently in all deployment environments. If it might be unset at runtime, consider implementing a fallback or error handling mechanism to prevent broken redirects.
- Verify that
process.env['CLIENT_BASE_URL']is set in production, staging, and other target environments.- If the variable might be undefined, add a fallback mechanism or additional logging to handle such cases gracefully.
packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts (10)
10-10: Added content type header
Explicitly setting'Content-Type': 'application/json'is a good improvement for clarity.
19-19: String-to-boolean conversion
This line correctly translates'isBillable'from Zapier’s string to a boolean for the API.
41-41: Minor description text
"For 'Starts a timer in Gauzy.'" This is consistent with the operation’s purpose.
45-52: Tenant selection field
Using adynamicproperty is a good approach for retrieving tenant IDs.
53-60: Organization selection field
Similarly consistent dynamic usage for organizations.
88-91: Default source mismatch
The AI summary suggests changing the default source to'ZAPIER', but the actual code retains'BROWSER'.Likely an incorrect or invalid review comment.
93-99: Added 'description' field
Optional text field is a valuable addition for specifying timer details.
100-107: Existing isBillable approach
Definingtype: 'boolean'with default'false'aligns with Zapier’s string-based boolean usage.
109-140: Extended dynamic fields
Extra fields for project, task, contact, and team. This is well-structured for capturing relevant context.
144-156: Sample updated
The new sample includes additional fields matching the updated input structure. The defaultsourceremains'BROWSER'in the sample, confirming the discrepancy with the summary.Likely an incorrect or invalid review comment.
packages/plugins/integration-zapier/zapier/src/triggers/organizationContactList.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/triggers/organizationTeamList.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/triggers/organizationTeamList.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/triggers/taskList.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/triggers/projectList.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-webhook.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/authentication.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts (1)
89-89: 🧹 Nitpick (assertive)Missing space after 'if' keyword.
There should be a space after the 'if' keyword for consistent code style.
- if(!clientId || !clientSecret) { + if (!clientId || !clientSecret) {packages/contracts/src/lib/zapier.model.ts (1)
23-27: 🧹 Nitpick (assertive)Interface structure looks good, consider adding JSDoc documentation.
The interface properly extends the base model and includes all necessary fields for timer webhook data. Consider adding JSDoc comments to document the purpose of each property for better code maintainability.
+/** + * Interface for timer status webhook data sent to Zapier + */ export interface ITimerZapierWebhookData extends IBasePerTenantAndOrganizationEntityModel { + /** The event name/type being triggered */ event: string; + /** Whether the timer is being started or stopped */ action: ActionType, + /** The time log data associated with this timer action */ data: ITimeLog; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
packages/contracts/src/lib/zapier.model.ts(2 hunks)packages/plugins/integration-zapier/src/lib/handlers/index.ts(1 hunks)packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-started.handler.ts(1 hunks)packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts(3 hunks)packages/plugins/integration-zapier/src/lib/zapier-webhook-subscription.entity.ts(1 hunks)packages/plugins/registry/src/lib/infrastructure/controllers/plugin-version.controller.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/contracts/src/lib/zapier.model.ts (2)
packages/contracts/src/index.ts (1)
IBasePerTenantAndOrganizationEntityModel(159-159)packages/contracts/src/lib/timesheet.model.ts (1)
ITimeLog(99-126)
packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-started.handler.ts (2)
packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-stopped.handler.ts (1)
Injectable(6-27)packages/plugins/integration-zapier/src/lib/zapier-webhook.service.ts (1)
Injectable(8-126)
packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts (1)
packages/desktop-core/src/lib/logger/logger.ts (1)
Logger(5-32)
🪛 Biome (1.9.4)
packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-started.handler.ts
[error] 23-23: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 24-24: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (14)
packages/plugins/integration-zapier/src/lib/zapier-webhook-subscription.entity.ts (2)
31-46: Good improvement to code consistency and documentation.The change to make the
integrationproperty optional with?correctly aligns the TypeScript type signature with the existing database configuration (nullable: true). The added detailed comments clearly document the relationship type and configuration, improving code readability and maintainability.
48-50: Documentation improvement.Adding descriptive comments for the
integrationIdproperty enhances code readability and maintainability, following the same documentation pattern as theintegrationproperty above.packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts (4)
3-3: Import statement updated correctly.The import statement has been updated to remove any unused imports while keeping the necessary ones. This is good practice for maintaining clean code.
77-77: Good addition of standardized logging.Adding a dedicated logger instance is a good practice that improves maintainability and consistent error handling throughout the plugin.
85-93: Excellent environment variable validation.Adding checks for the required OAuth credentials provides early warning of misconfiguration and improves the developer experience. The conditional logging is informative and appropriately uses warn/log levels.
102-102: Good use of the logger instance.Replacing direct console logging with the logger instance improves consistency and maintainability.
packages/contracts/src/lib/zapier.model.ts (1)
21-21: Well-defined type for timer actions.The
ActionTypetype clearly defines the allowed timer actions ('start' or 'stop'), providing type safety for the webhook data structure.packages/plugins/registry/src/lib/infrastructure/controllers/plugin-version.controller.ts (2)
57-57: Formatting change looks good.Minor formatting improvement with the added space before the closing brace.
157-157: Fixed typo in API documentation.Corrected "data transfert object" to "data transfer object" in the API description, improving documentation accuracy.
packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-started.handler.ts (4)
1-5: Import statements look good!The necessary dependencies for event handling and Zapier webhook service are correctly imported.
6-8: Class setup follows best practicesThe class is properly decorated with
@Injectable()for dependency injection and@EventsHandler(TimerStartedEvent)to specify which event it handles. It correctly implements theIEventHandler<TimerStartedEvent>interface.
9-9: Constructor injection is implemented correctlyDependency injection through the constructor follows NestJS best practices.
11-16: Well-documented method with JSDocThe JSDoc comments clearly explain the purpose of the method, its parameters, and return value.
packages/plugins/integration-zapier/src/lib/handlers/index.ts (1)
1-2: Imports are correctly structuredThe handler imports are appropriately defined.
packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-started.handler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (2)
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (2)
27-84: 🛠️ Refactor suggestionConsider validating the domain of the redirect URI.
Currently, the
authorizemethod ensures theredirectUriparameter is present but does not verify whether it falls within a trusted set of domains. For security, particularly in OAuth2 flows, consider checking thatzapier_redirect_uriis a permitted domain to mitigate potential open-redirect vulnerabilities.
87-153: 🧹 Nitpick (assertive)Consider verifying fallback redirect.
When
zapier_redirect_uriis undefined, the method falls back toZAPIER_REDIRECT_URI. Ensure that it is acceptable for all scenarios or restrict usage to known safe domains.
♻️ Duplicate comments (3)
packages/plugins/integration-zapier/src/lib/handlers/index.ts (1)
1-5: Event handlers export is well-organizedThe export of handlers as an array follows good module organization practices, making it easy to register all handlers at once.
For consistency with the rest of the codebase, consider adding a semicolon at the end of the statement:
-export const EventHandlers = [ZapierTimerStartedHandler, ZapierTimerStoppedHandler] +export const EventHandlers = [ZapierTimerStartedHandler, ZapierTimerStoppedHandler];packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-started.handler.ts (1)
19-21: 🛠️ Refactor suggestionAdd early return to prevent processing with missing IDs
The code warns when
tenantIdororganizationIdis missing, but still attempts to call the service method with potentially undefined values. This could lead to downstream errors.if (!timeLog.tenantId || !timeLog.organizationId) { console.warn('Cannot process timer started event: missing tenantId or organizationId') + return; }packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (1)
50-83: 🧹 Nitpick (assertive)Suggest concurrency safeguards for high volume.
When the auth code limit is reached, you trigger an on-demand cleanup, which works for moderate usage. However, if usage spikes concurrently, consider mutex-based locking to avoid potential race conditions on entering this cleanup logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (22)
.env.compose(1 hunks).env.demo.compose(1 hunks).env.docker(1 hunks).env.local(1 hunks).env.sample(1 hunks)packages/common/src/lib/interfaces/IZapierConfig.ts(1 hunks)packages/config/src/lib/config/zapier.ts(1 hunks)packages/config/src/lib/environments/environment.prod.ts(1 hunks)packages/config/src/lib/environments/environment.ts(1 hunks)packages/core/src/lib/auth/auth.controller.ts(0 hunks)packages/plugins/integration-zapier/src/lib/handlers/index.ts(1 hunks)packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-started.handler.ts(1 hunks)packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-stopped.handler.ts(1 hunks)packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts(2 hunks)packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts(4 hunks)packages/plugins/integration-zapier/src/lib/zapier.controller.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier.service.ts(5 hunks)packages/plugins/integration-zapier/zapier/package.json(1 hunks)packages/plugins/integration-zapier/zapier/src/authentication.ts(1 hunks)packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts(4 hunks)packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts(4 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/lib/auth/auth.controller.ts
🧰 Additional context used
🧠 Learnings (3)
packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts (1)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts:100-107
Timestamp: 2025-04-10T15:31:05.841Z
Learning: In Zapier integrations, boolean configuration fields need to use type 'boolean' but with string default values like 'true' or 'false' because that's what the Zapier platform expects to receive. The actual boolean conversion is handled in the code with a comparison like `inputData['fieldName'] === 'true'`.
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (3)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:6-19
Timestamp: 2025-04-11T06:13:47.856Z
Learning: For the Zapier integration auth code storage, using an in-memory Map is acceptable for moderate usage volumes due to the built-in safeguards: a maximum limit of 1000 codes and an automatic cleanup mechanism for expired codes.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:93-105
Timestamp: 2025-04-11T09:03:32.978Z
Learning: For the Zapier integration auth code storage, on-demand cleanup (triggered when approaching the maximum code limit) is sufficient without the need for periodic scheduled cleanup, as determined by the implementation requirements.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:37-70
Timestamp: 2025-04-11T14:06:04.523Z
Learning: The ZapierAuthCodeService unit tests will be implemented in a future PR, as confirmed by RolandM99.
packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts (2)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts:100-107
Timestamp: 2025-04-10T15:31:05.841Z
Learning: In Zapier integrations, boolean configuration fields need to use type 'boolean' but with string default values like 'true' or 'false' because that's what the Zapier platform expects to receive. The actual boolean conversion is handled in the code with a comparison like `inputData['fieldName'] === 'true'`.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8863
File: packages/plugins/integration-zapier/zapier/src/authentication.ts:29-29
Timestamp: 2025-04-02T09:39:39.648Z
Learning: When developing Zapier integrations that will be deployed to the Zapier platform, use `process.env` directly to access environment variables instead of importing from `@gauzy/config`, as Zapier's platform cannot recognize or import this package.
🧬 Code Graph Analysis (3)
packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts (1)
packages/desktop-core/src/lib/logger/logger.ts (1)
Logger(5-32)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (1)
packages/desktop-core/src/lib/logger/logger.ts (1)
Logger(5-32)
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (6)
packages/desktop-core/src/lib/logger/logger.ts (1)
Logger(5-32)packages/core/src/lib/core/context/request-context.ts (1)
RequestContext(16-439)packages/plugins/integration-zapier/src/lib/zapier.config.ts (2)
ZAPIER_REDIRECT_URI(14-14)ZAPIER_TOKEN_EXPIRATION_TIME(17-17)packages/common/src/lib/decorators/public.decorator.ts (1)
Public(15-15)packages/plugins/integration-zapier/src/lib/dto/create-zapier-integration.dto.ts (1)
CreateZapierIntegrationDto(6-31)packages/contracts/src/lib/zapier.model.ts (2)
IZapierAccessTokens(4-9)ZapierGrantType(11-11)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (35)
packages/plugins/integration-zapier/zapier/package.json (1)
16-16: Dependency updates for Zapier platform libraries.The version updates to
zapier-platform-coreandzapier-platform-clifrom 16.3.1 to 16.5.0 are appropriate. These updates ensure the plugin benefits from the latest features, bug fixes, and security patches from the Zapier platform libraries.Also applies to: 22-22
packages/common/src/lib/interfaces/IZapierConfig.ts (1)
11-13:✅ Verification successful
Good addition of domain validation for OAuth security.
Adding the
allowedDomainsproperty to the interface is a solid security enhancement that will help prevent open redirect vulnerabilities in the OAuth flow by restricting redirect URIs to a predefined list of trusted domains.
🏁 Script executed:
#!/bin/bash # Check if this new interface property is implemented properly in the authorization code # Search for code that validates redirect URIs against the allowed domains rg -A 10 -B 5 "allowedDomains" --type tsLength of output: 7234
Security Enhancement Verified: The new
allowedDomainsproperty added inpackages/common/src/lib/interfaces/IZapierConfig.tsis properly integrated. The grep output confirms that the domain validation logic is implemented in the authorization code (e.g., inpackages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts) and the configuration files (both environment files andpackages/config/src/lib/config/zapier.ts) correctly parse and use this property to restrict OAuth redirect URIs. Great work on improving the OAuth flow security!.env.compose (1)
152-152: Security improvement with domain allowlist.The addition of the
ALLOWED_DOMAINSenvironment variable with a curated list of trusted domains is a good practice for securing OAuth flows. This prevents potential redirect attacks by limiting the destinations to which users can be redirected during the authentication process..env.demo.compose (1)
154-154: Security enhancement for OAuth flowThe addition of the
ALLOWED_DOMAINSenvironment variable implements a good security practice by restricting which domains can be used for OAuth redirects, preventing potential open redirect vulnerabilities..env.local (1)
168-168: Consistent security configuration across environmentsThe addition of the
ALLOWED_DOMAINSvariable provides consistent security configuration with other environment files, ensuring proper domain validation during OAuth flows regardless of deployment environment.packages/config/src/lib/environments/environment.ts (1)
196-196:❓ Verification inconclusive
Well-implemented configuration for allowed domains
The addition of the
allowedDomainsproperty correctly processes the environment variable into a usable array format, following good practices for environment variable handling with appropriate fallback to an empty array.Note that this configuration works with the
GAUZY_ALLOWED_DOMAINSenvironment variable, while the.envfiles setALLOWED_DOMAINS. Make sure there's proper mapping between these variables in your configuration system.
🏁 Script executed:
#!/bin/bash # Verify if there's a mapping between ALLOWED_DOMAINS and GAUZY_ALLOWED_DOMAINS # Check for any code that reads ALLOWED_DOMAINS or maps it to GAUZY_ALLOWED_DOMAINS rg "ALLOWED_DOMAINS" --type tsLength of output: 1196
Attention: Allowed Domains Configuration Verification
The implementation in
packages/config/src/lib/environments/environment.tscorrectly processes theGAUZY_ALLOWED_DOMAINSenvironment variable into an array. However, note that while the code expectsGAUZY_ALLOWED_DOMAINS, the.envfiles setALLOWED_DOMAINS. Our repository search shows references toALLOWED_DOMAINS(for example, in the Zapier integration) but does not reveal any explicit mapping between the two names.
- Action Required: Please confirm that your overall configuration system (or deployment setup) properly maps the
.envvariableALLOWED_DOMAINStoGAUZY_ALLOWED_DOMAINS. If no such mapping exists, you might need to either update the environment variable used in the.envfiles or add the missing translation in your configuration logic.packages/config/src/lib/config/zapier.ts (1)
17-18: Implemented clean domain handling logic.The addition of
allowedDomainscreates a whitelist mechanism for OAuth redirect URIs, adding an important security layer. The implementation properly handles string splitting and trims each domain to prevent issues with whitespace..env.docker (1)
151-151: Verify all necessary domains are included.Good addition of the allowed domains for OAuth flow. This whitelist includes essential domains for integrations. Consider adding a comment indicating this is for the Zapier OAuth integration security constraints.
Please confirm that the environment variable in the code (
GAUZY_ALLOWED_DOMAINS) matches what's defined here (ALLOWED_DOMAINS). There seems to be a prefix discrepancy between the config usage and environment variable definition.packages/plugins/integration-zapier/zapier/src/authentication.ts (4)
4-5: Good use of environment variable for application name.The code properly uses an environment variable for the application name with a sensible default, which supports white-labeling capabilities.
11-17: Improved authentication test endpoint implementation.The test configuration now properly specifies the HTTP method and includes the correct authorization header format, which will ensure proper authentication verification.
19-41: Well-implemented connection label with robust error handling.The connectionLabel function effectively fetches user data and formats the label with proper error handling. The implementation also uses optional chaining for safer property access as recommended in past reviews.
43-88: Authentication configuration is correctly structured.The OAuth2 configuration correctly defines the authorization, token generation, and refresh flows with appropriate parameters and headers.
packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-started.handler.ts (1)
1-30: Implementation looks good for handling timer started eventsThe handler correctly listens for
TimerStartedEventevents and uses the webhook service to notify Zapier. The implementation validates required IDs and follows the event-driven architecture pattern well.packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-stopped.handler.ts (1)
1-30: Implementation handles timer stopped events correctlyThe handler properly listens for
TimerStoppedEventevents and uses the webhook service to notify Zapier with the appropriate event and action parameters.packages/plugins/integration-zapier/src/lib/integration-zapier.plugin.ts (1)
75-93: Improved configuration management and loggingThe changes nicely replace direct console logging with a proper logger instance and use ConfigService to access configuration values. The code now correctly checks for required OAuth credentials and logs appropriate warnings.
packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts (3)
30-33: Enhanced error handling looks goodThe error handling improvements provide better debugging context and clearer error messages for users.
47-142: Dynamic dropdowns and improved field documentationThe input fields now use dynamic dropdowns for entity selection and include helpful descriptions, which will significantly improve the user experience in Zapier.
3-3: Good use of environment variable with fallbackUsing
process.env.APP_NAME || 'Gauzy'ensures the display will work properly in all environments.packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (3)
1-5: Imports are clear and well-structured.All necessary NestJS dependencies and third-party modules are properly referenced.
12-29: Good handling of allowed domains via config.Merging the default domains with environment-specified domains is a flexible approach. The logging of allowed domains aids in debugging.
120-139: Periodic cleanup complements on-demand approach.Running cleanup on a timer plus on-demand helps prevent unbounded growth in edge scenarios. This aligns with your moderate-volume design choice.
packages/plugins/integration-zapier/src/lib/zapier.service.ts (2)
5-30: Imports and logger usage look good.Bringing in
ZAPIER_TOKEN_EXPIRATION_TIMEand initializing a dedicatedLoggerfor this service is consistent with best practices.
147-234: createIntegration method logic is coherent.Using
uuidv4to generate tokens is straightforward. The command bus approach to initialize entity settings is clean. Ensure tokens are stored securely if any compliance requirements arise.packages/plugins/integration-zapier/src/lib/zapier.controller.ts (5)
4-19: Extended imports and decorators.Adding
BadRequestException,HttpStatus, and the config injection sets a solid foundation for OAuth endpoints. The permission guards are also suitably placed.
72-89: getTriggers method is concise.Nice approach delegating HTTP interactions to
ZapierService. The separate error handler helps maintain consistent responses.
91-108: getActions method parallels getTriggers.Reusing
validateTokenwith a different unauthorized condition is a clean design choice. The method’s flow is clear and consistent.
113-121: validateToken handles missing vs. empty tokens.Splitting between
BadRequestExceptionandUnauthorizedExceptionbased on usage context is a neat pattern for more fine-grained error responses.
126-144: Centralized error handling in handleZapierError.Re-throwing known errors and wrapping unknown ones in
InternalServerErrorExceptionkeeps error responses uniform. This is a solid approach for maintainability.packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (2)
1-24: Imports and constructor injection appear consistent.No syntax or logical issues found in these declarations. The setup for
_config,ZapierAuthCodeService, andZapierServiceis correct and follows standard NestJS patterns.
220-289: Good practice masking refresh tokens in logs.The
refreshTokenmethod effectively avoids disclosing sensitive token details by masking them in the debug logs. No issues found here; this is a solid security approach.packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts (5)
3-4: Environment-based app name and updated request headers look good.Using
APP_NAMEfromprocess.envfor the label/description is consistent with Zapier deployment requirements, and the Bearer token header is correctly formed.Also applies to: 11-13
21-21: Boolean conversion for isBillable is appropriate.By checking
bundle.inputData['isBillable'] === 'true', the code aligns with Zapier's practice of sending booleans as strings. This matches your retrieval logic and avoids type mismatches.
43-43: Excellent use of a dynamic application name.Referencing
appNamein the description underscores branding consistency when the integration name changes.
146-159: Sample data updated for demonstration.The modified sample reflects the new fields (e.g.,
description,projectId, etc.) and offers realistic UUIDs. This consistency helps users correctly configure the Zap. No concerns here.
47-142:❓ Verification inconclusive
Verify dynamic dropdown triggers.
You introduced dynamic inputs for tenants, organizations, and other entities by referencing keys like
tenant_list.id.name. Confirm that these triggers are correctly defined in your Zapier app so the dropdowns populate properly at runtime.
🏁 Script executed:
#!/bin/bash # Verify the existence of trigger definitions for the dynamic fields rg "tenant_list\|organization_list\|project_list\|task_list\|organization_contact_list\|organization_team_list" -A 6Length of output: 118
Action Required: Confirm Dynamic Trigger Definitions for Dropdowns
The grep search for trigger definitions (e.g. "tenant_list", "organization_list", etc.) yielded no results, so it’s unclear if these dynamic dropdown triggers are defined elsewhere in the Zapier app configuration. Please verify manually that the dynamic trigger identifiers referenced in this file are correctly implemented so the dropdowns populate properly at runtime:
- Confirm definitions exist for:
tenant_list.id.nameorganization_list.id.nameproject_list.id.nametask_list.id.nameorganization_contact_list.id.nameorganization_team_list.id.name
packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-stopped.handler.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier.controller.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (2)
packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts (1)
78-93:⚠️ Potential issueFix inconsistency in source field choices.
The source options don't include 'ZAPIER', yet the integration is for Zapier. The default is set to 'BROWSER', which seems inconsistent with the use case.
choices: { BROWSER: 'Browser', DESKTOP: 'Desktop', MOBILE: 'Mobile', BROWSER_EXTENSION: 'Browser Extension', HUBSTAFF: 'Hubstaff', UPWORK: 'Upwork', TEAMS: 'Teams', - CLOC: 'Cloc' + CLOC: 'Cloc', + ZAPIER: 'Zapier' }, - default: 'BROWSER', + default: 'ZAPIER', helpText: 'The source of this timer entry'packages/plugins/integration-zapier/src/lib/zapier.controller.ts (1)
122-130: 🧹 Nitpick (assertive)Parameter naming tip.
The
isThrowUnauthorizedOnMissingTokenparameter might be more explicit with a name likethrowUnauthorized. Shortening improves readability while preserving intent.
♻️ Duplicate comments (3)
packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts (1)
146-159: 🧹 Nitpick (assertive)Update sample data for consistency.
The sample data shows
source: 'BROWSER'but this doesn't align with the expected source for a Zapier integration.id: '55a664ca-7266-4e71-b37a-dfc1fe824478', tenantId: '88507509-f7cb-44f4-ae60-894f950477a9', organizationId: 'b894c374-7374-43cb-a1a2-afe46b9f5e28', sentTo: 'da2117d6-e6f6-45ec-86cb-80cc20470ba4', logType: 'TRACKED', - source: 'BROWSER', + source: 'ZAPIER', description: 'Timer started via Zapier integration',.env.sample (1)
156-157: Security improvement with domain allowlist in sample config.The addition is well-documented and ensures developers include this security configuration. No further changes needed.
packages/config/src/lib/environments/environment.prod.ts (1)
196-196: Domain trimming for allowedDomains is correctly implemented.The use of
.map(domain => domain.trim())ensures consistent and robust parsing of the allowed domains list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
.env.compose(1 hunks).env.demo.compose(1 hunks).env.docker(1 hunks).env.local(1 hunks).env.sample(1 hunks)packages/config/src/lib/environments/environment.prod.ts(1 hunks)packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-stopped.handler.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts(4 hunks)packages/plugins/integration-zapier/src/lib/zapier.controller.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier.service.ts(5 hunks)packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (4)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:6-19
Timestamp: 2025-04-11T06:13:47.856Z
Learning: For the Zapier integration auth code storage, using an in-memory Map is acceptable for moderate usage volumes due to the built-in safeguards: a maximum limit of 1000 codes and an automatic cleanup mechanism for expired codes.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:93-105
Timestamp: 2025-04-11T09:03:32.978Z
Learning: For the Zapier integration auth code storage, on-demand cleanup (triggered when approaching the maximum code limit) is sufficient without the need for periodic scheduled cleanup, as determined by the implementation requirements.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:37-70
Timestamp: 2025-04-11T14:06:04.523Z
Learning: The ZapierAuthCodeService unit tests will be implemented in a future PR, as confirmed by RolandM99.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:145-165
Timestamp: 2025-04-15T13:43:30.394Z
Learning: The domain validation in ZapierAuthCodeService doesn't need to handle IP addresses, as OAuth redirects typically only require domain name validation.
packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts (2)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/zapier/src/creates/stopTimer.ts:100-107
Timestamp: 2025-04-10T15:31:05.841Z
Learning: In Zapier integrations, boolean configuration fields need to use type 'boolean' but with string default values like 'true' or 'false' because that's what the Zapier platform expects to receive. The actual boolean conversion is handled in the code with a comparison like `inputData['fieldName'] === 'true'`.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8863
File: packages/plugins/integration-zapier/zapier/src/authentication.ts:29-29
Timestamp: 2025-04-02T09:39:39.648Z
Learning: When developing Zapier integrations that will be deployed to the Zapier platform, use `process.env` directly to access environment variables instead of importing from `@gauzy/config`, as Zapier's platform cannot recognize or import this package.
🧬 Code Graph Analysis (1)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (1)
packages/desktop-core/src/lib/logger/logger.ts (1)
Logger(5-32)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (21)
packages/plugins/integration-zapier/src/lib/handlers/zapier-timer-stopped.handler.ts (1)
1-31: LGTM! Well-structured event handler with proper error handling.The implementation correctly handles the timer stopped event and notifies the Zapier webhooks. Good job on including early return when required IDs are missing.
packages/plugins/integration-zapier/zapier/src/creates/startTimer.ts (4)
3-3: Good use of environment variable with fallback.Using
process.env.APP_NAMEwith fallback to 'Gauzy' follows best practices for configuration management.
30-32: Good error handling using Zapier's error API.The updated error handling correctly uses Zapier's
z.errors.HaltedErrorfor better error reporting in the Zapier platform.
103-109: Appropriate use of boolean field with string values for Zapier compatibility.The implementation correctly uses
'true'/'false'string values for the boolean field to comply with Zapier platform requirements, with proper conversion in the code.
47-142: Great improvement with dynamic dropdowns for related entities.The added input fields with dynamic dropdowns greatly improve the user experience by allowing selection of related entities like tenants, organizations, projects, tasks, etc., instead of requiring IDs to be entered manually.
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (7)
6-29: Well-structured initialization of allowed domains with appropriate defaults.The service properly initializes with domains from configuration while ensuring localhost is always included for development. Using Set to deduplicate domains and trimming values demonstrates good attention to detail. The logging of allowed domains will be helpful for troubleshooting.
31-39: In-memory storage with comprehensive code metadata.The Map-based temporary storage is appropriate for managing short-lived authorization codes. The code structure includes all necessary metadata (user ID, tenant ID, expiration, etc.) to securely manage the OAuth flow.
50-83: Robust auth code generation with appropriate security checks.The implementation includes proper redirect URI validation, expiration time setting, and code limit enforcement. The method handles edge cases well by cleaning up expired codes when approaching the limit.
122-130: Effective on-demand cleanup implementation.The cleanup mechanism properly removes expired codes and includes debug logging for each removal. This approach aligns with the learnings from previous discussions about not needing scheduled cleanups due to the implementation requirements.
135-141: Periodic cleanup adds reliability alongside on-demand cleanup.The periodic cleanup provides an additional layer of protection by regularly removing expired codes, complementing the on-demand cleanup triggered when approaching the code limit. The 5-minute interval is a reasonable choice for this use case.
147-167: Comprehensive redirect URI domain validation.The implementation handles various domain scenarios well, including explicit localhost support for development and proper error catching for malformed URIs. The domain matching logic correctly handles both exact matches and subdomains.
172-187: Utility method for dynamic domain detection.The
getServerDomainFromRequestmethod provides a useful way to determine the current server domain, which could be used for dynamic domain validation. The implementation correctly handles edge cases and includes proper error handling.packages/plugins/integration-zapier/src/lib/zapier.service.ts (2)
1-30: Use consistent constants and consider config-based token expiration.Lines 1, 5, 17, 26, and 30 introduce new imports, the
uuidv4function, theIZapierAccessTokensinterface, aZAPIER_TOKEN_EXPIRATION_TIMEconstant, and a logger instance. Everything looks good. In future refactors, consider loading token expiration time from a dedicated config service, allowing easy changes without code modifications.
154-241:✅ Verification successful
Validate existence of integration before using it.
In
createIntegration(lines 154–241), you fetch an integration withfindOneByOptionson line 162, but if it’sundefinedyou just passundefinedto theIntegrationTenantUpdateOrCreateCommand. Consider handling the scenario where the integration isn’t registered at all, providing a more descriptive error or fallback.Run this script to confirm whether an undefined integration triggers any errors downstream:
🏁 Script executed:
#!/bin/bash rg -A 5 'IntegrationTenantUpdateOrCreateCommand'Length of output: 11479
Integration Existence Validation: Consistent Behavior Confirmed
After reviewing the grep output across multiple integration modules (e.g., Upwork, Hubstaff, and GitHub), it’s clear that passing a potentially undefined integration (as fetched via
_integrationService.findOneByOptions) is consistently handled by theIntegrationTenantUpdateOrCreateCommand. The command is always constructed with default options (including a provider object) while the possibly undefined integration is forwarded in the second parameter. This pattern appears intentional, as it likely signals the command to create a new integration tenant when one isn’t already registered.No changes are required unless you want to add explicit error handling. If the intent was to introduce a fallback or a descriptive error when the integration isn’t found, that behavior would need to be implemented consistently across the codebase. Otherwise, the current approach is in line with how other integration services operate.
packages/plugins/integration-zapier/src/lib/zapier.controller.ts (2)
4-19: Imports and decorators look consistent.The added imports/decorators (lines 4, 11–12, 14–15, 18–19) align with the new endpoints and token validation approach. No issues found.
81-98:✅ Verification successful
Confirm token usage consistency.
The
getTriggersmethod (lines 81–98) validates the token (line 92) and fetches triggers from the service. Looks good. Ensure all references tofetchTriggersacross the codebase pass the same token to avoid partial authentication confusion.
🏁 Script executed:
#!/bin/bash rg "fetchTriggers" -A 3Length of output: 865
Token usage is consistent across the controller and service layers.
The grep output confirms that the token passed tofetchTriggersin the controller is consistently propagated to the service method and further used infetchIntegration. No discrepancies were found.packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (3)
42-77: Redirect logic and input validation.
- The
authorizemethod (lines 42–77) ensuresresponse_type === 'code'. Good.- You log debug statements for
redirectUriandstate. Consider redacting or limiting logs ifredirectUrimight contain sensitive data.
108-145: Consider domain validation consistency.The
callbackmethod (lines 108–145) checks for azapier_redirect_uribut does not explicitly verify its domain like inloginSuccess(zapier.controller.ts). Maintaining consistent domain checks ensures better security.
227-294: Refresh endpoint feedback.The
refreshTokenmethod (lines 227–294) logs the request with a maskedrefresh_token. Good job obscuring sensitive data. IfintegrationIdis invalid or tokens are missing, the method correctly throws exceptions. Everything looks consistent, so no further changes are needed here..env.compose (1)
152-153: Addition ofALLOWED_DOMAINSfor OAuth security is clear and well-documented.This environment variable is properly documented and aligns with best practices for securing OAuth redirect URIs.
.env.local (1)
168-169: Addition ofALLOWED_DOMAINSfor OAuth security is clear and well-documented.This environment variable is properly documented and aligns with best practices for securing OAuth redirect URIs.
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier.controller.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier.controller.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
packages/plugins/integration-zapier/src/lib/zapier.service.ts (1)
154-247: 🧹 Nitpick (assertive)Confirm entity settings integrity in “createIntegration” and consider partial failures.
- The logic that maps default entity settings (lines 178–195) and then passes them into
IntegrationTenantUpdateOrCreateCommandis sound for establishing a new Zapier integration. However, ensure that partial creation failures (e.g., due to DB constraints) are handled gracefully, or that a valid transaction boundary is in place.- For expansions, such as adding more custom settings or additional tokens, consider verifying if the new settings are unique to avoid overwriting existing settings if the user calls
createIntegrationmultiple times.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.env.demo.compose(1 hunks).env.docker(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts(4 hunks)packages/plugins/integration-zapier/src/lib/zapier.controller.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier.service.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (4)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:6-19
Timestamp: 2025-04-11T06:13:47.856Z
Learning: For the Zapier integration auth code storage, using an in-memory Map is acceptable for moderate usage volumes due to the built-in safeguards: a maximum limit of 1000 codes and an automatic cleanup mechanism for expired codes.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:93-105
Timestamp: 2025-04-11T09:03:32.978Z
Learning: For the Zapier integration auth code storage, on-demand cleanup (triggered when approaching the maximum code limit) is sufficient without the need for periodic scheduled cleanup, as determined by the implementation requirements.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:37-70
Timestamp: 2025-04-11T14:06:04.523Z
Learning: The ZapierAuthCodeService unit tests will be implemented in a future PR, as confirmed by RolandM99.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:145-165
Timestamp: 2025-04-15T13:43:30.394Z
Learning: The domain validation in ZapierAuthCodeService doesn't need to handle IP addresses, as OAuth redirects typically only require domain name validation.
🧬 Code Graph Analysis (1)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (1)
packages/desktop-core/src/lib/logger/logger.ts (1)
Logger(5-32)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (9)
.env.demo.compose (1)
154-155: Looks good!
The addedALLOWED_DOMAINSenvironment variable aligns with best practices for restricting OAuth redirect domains. No issues identified here.packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (4)
91-120: Code validity check logic is well-handled.
Deleting the code upon successful validation promotes one-time usage. The logging for mismatches and expired codes aids troubleshooting.
132-142: Periodic cleanup is well-structured.
Running a 5-minute interval to remove expired codes ensures memory is freed even without frequent requests. This complements your on-demand cleanup ingenerateAuthCode.
144-167: Redirect domain validation is thorough.
Allowinglocalhostand matching subdomains from an allowed list is an effective approach. This helps prevent unauthorized redirect targets.
168-188: Server domain helper is a helpful fallback.
Extracting the domain from the Host header is a neat option for scenarios where the environment variable may be unavailable.packages/plugins/integration-zapier/src/lib/zapier.service.ts (1)
30-30: Good addition of a dedicated logger instance.Defining
private readonly logger = new Logger(ZapierService.name);helps maintain clear, class-scoped logging that can differentiate messages from this service versus other services.packages/plugins/integration-zapier/src/lib/zapier.controller.ts (2)
49-108: Domain validation approach looks good.
- Properly checking
zapier_redirect_uriagainst a list of authorized domains/subdomains is crucial for security.- The fallback redirect to
/dashboardwhen neitherzapier_statenorzapier_redirect_uriis provided helps avoid user confusion.- Consider additional logging or custom exceptions if more nuance is needed, but overall this flow is straightforward and secure.
155-163: Clear token validation helper.The
validateTokenmethod correctly handles missing or empty tokens, throwing eitherBadRequestExceptionorUnauthorizedExceptionbased on theisThrowUnauthorizedOnMissingTokenflag. This is a concise and readable design.packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (1)
25-50: Early configuration validation is a smart move.Verifying the presence and correctness of critical environment variables (like Zapier
clientId,clientSecret, and allowed domains) inonModuleInit()helps catch misconfigurations before runtime. Good practice.
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier.controller.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
.env.docker (1)
151-154: 🛠️ Refactor suggestionRemove leading spaces in environment variable definitions.
The leading spaces before values on lines 153-154 might cause parsing issues in some environments. Environment variables should be defined without leading spaces to ensure consistent behavior across platforms.
# Comma-separated list of domains allowed for OAuth redirects (security feature) - ALLOWED_DOMAINS=gauzy.co,ever.co,staging.gauzy.co,demo.gauzy.co,zapier.com,github.com,upwork.com,hubstaff.com,fiverr.com - MAX_AUTH_CODES=1000 +ALLOWED_DOMAINS=gauzy.co,ever.co,staging.gauzy.co,demo.gauzy.co,zapier.com,github.com,upwork.com,hubstaff.com,fiverr.com +MAX_AUTH_CODES=1000.env.compose (1)
152-154: 🛠️ Refactor suggestionRemove leading spaces in environment variables.
The leading spaces before the values on lines 153-154 should be removed to prevent potential parsing issues in certain environments.
# Comma-separated list of domains allowed for OAuth redirects (security feature) - ALLOWED_DOMAINS=gauzy.co,ever.co,staging.gauzy.co,demo.gauzy.co,zapier.com,github.com,upwork.com,hubstaff.com,fiverr.com - MAX_AUTH_CODES=1000 +ALLOWED_DOMAINS=gauzy.co,ever.co,staging.gauzy.co,demo.gauzy.co,zapier.com,github.com,upwork.com,hubstaff.com,fiverr.com +MAX_AUTH_CODES=1000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
.env.compose(1 hunks).env.demo.compose(1 hunks).env.docker(1 hunks).env.local(1 hunks).env.sample(1 hunks)packages/common/src/lib/interfaces/IZapierConfig.ts(1 hunks)packages/config/src/lib/environments/environment.prod.ts(1 hunks)packages/config/src/lib/environments/environment.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts(4 hunks)packages/plugins/integration-zapier/src/lib/zapier.controller.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier.service.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (5)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:27-29
Timestamp: 2025-04-15T18:29:09.300Z
Learning: The ZapierAuthCodeService in the Zapier integration plugin is designed to be initialized only once as a singleton service in the NestJS application.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:6-19
Timestamp: 2025-04-11T06:13:47.856Z
Learning: For the Zapier integration auth code storage, using an in-memory Map is acceptable for moderate usage volumes due to the built-in safeguards: a maximum limit of 1000 codes and an automatic cleanup mechanism for expired codes.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:93-105
Timestamp: 2025-04-11T09:03:32.978Z
Learning: For the Zapier integration auth code storage, on-demand cleanup (triggered when approaching the maximum code limit) is sufficient without the need for periodic scheduled cleanup, as determined by the implementation requirements.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:37-70
Timestamp: 2025-04-11T14:06:04.523Z
Learning: The ZapierAuthCodeService unit tests will be implemented in a future PR, as confirmed by RolandM99.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:145-165
Timestamp: 2025-04-15T13:43:30.444Z
Learning: The domain validation in ZapierAuthCodeService doesn't need to handle IP addresses, as OAuth redirects typically only require domain name validation.
🧬 Code Graph Analysis (1)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (1)
packages/desktop-core/src/lib/logger/logger.ts (1)
Logger(5-32)
🪛 Biome (1.9.4)
packages/config/src/lib/environments/environment.ts
[error] 197-197: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
packages/config/src/lib/environments/environment.prod.ts
[error] 197-197: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (9)
.env.sample (1)
156-158: Security enhancement with domain restriction and auth code limits.Adding these environment variables strengthens the security of OAuth implementation:
ALLOWED_DOMAINSrestricts redirect URIs to trusted domains onlyMAX_AUTH_CODESlimits the number of auth codes that can be issuedThe comment provides good context on the security purpose of the domain restrictions.
.env.demo.compose (1)
154-156: Security enhancement for OAuth redirectsThese new environment variables strengthen the Zapier OAuth2 implementation by:
- Restricting redirect URIs to a specific set of trusted domains
- Limiting the number of authentication codes to prevent abuse
The whitelist approach for domains is a best practice for OAuth security.
packages/common/src/lib/interfaces/IZapierConfig.ts (2)
11-13: Well-documented domain validation propertyThe
allowedDomainsproperty adds an important security layer to the Zapier OAuth integration by restricting redirect URIs to approved domains only.
20-21: Good addition of auth code limitAdding a configurable limit for authentication codes helps prevent potential resource exhaustion attacks.
.env.local (1)
168-170: Security enhancement for OAuth redirectsAdding these environment variables improves security by restricting redirect URIs to trusted domains and limiting the number of authentication codes that can be issued.
packages/config/src/lib/environments/environment.prod.ts (1)
196-197: Domain verification and auth code limits properly implementedThe implementation correctly reads the allowed domains from environment variables, splits the comma-separated list, and trims each domain to handle potential whitespace. The auth code limit defaults to 1000 if not specified.
🧰 Tools
🪛 Biome (1.9.4)
[error] 197-197: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
packages/plugins/integration-zapier/src/lib/zapier.service.ts (2)
125-133: Wrap unknown errors consistently.
This block re-throws known exceptions and then generically wraps others in anHttpException. This is a solid approach for consistent error responses.
100-108:❓ Verification inconclusive
Potential race conditions during token update.
The map operation with updated settings is straightforward, but if multiple calls to refresh tokens occur simultaneously, you could see conflicting updates. For moderate loads, this is likely fine, but keep concurrency in mind if scaling increases.Do you want to verify if any concurrency issues might arise by checking other references to
refreshTokenor concurrency patterns using a quick search?
🏁 Script executed:
#!/bin/bash # Scans for concurrency patterns or parallel refresh calls in the codebase rg "refreshToken" -A 10Length of output: 113089
Action: Review concurrency handling in token refresh for Zapier integration
The updated review comment reflects that while the current approach of mapping and updating token settings is simple and acceptable for moderate loads, it may lead to race conditions if the refresh calls happen concurrently. Notably, other integrations (e.g., the Hubstaff interceptor in the ui-core package) implement explicit concurrency controls using flags and observable subjects. Please verify whether a similar safeguard is necessary for the Zapier integration, especially if concurrent token refreshes are expected as load increases.
- Location:
packages/plugins/integration-zapier/src/lib/zapier.service.ts(Lines 100–108)- Observation: The code updates token settings using a mapping operation without any concurrency protection.
- Recommendation: Evaluate if—and under what circumstances—you might need to implement an atomic or concurrency-managed update (similar to the approach in
packages/ui-core/core/src/lib/interceptors/hubstaff-token-interceptor.ts).packages/plugins/integration-zapier/src/lib/zapier.controller.ts (1)
55-85: Domain validation is well implemented.
Your checks for localhost and configured domains (including subdomain handling) are thorough. Logging the unauthorized URI along with the allowed domains is a helpful diagnostic.
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier.controller.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier.controller.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (1)
78-210: 🛠️ Refactor suggestionImplement state parameter verification for OAuth2
The current flow does not verify whether thestateparameter in the callback matches the one specified inauthorize(), which is a recommended step to mitigate CSRF or injection attacks. Standard OAuth2 flows store thestatein a server-side session or cache and verify it upon callback.Consider persisting the
statebefore the redirect and validating it afterward:// In authorize(): + this.someTemporaryStore.set(state, true); // In callback(): + if (!this.someTemporaryStore.get(state)) { + throw new HttpException('Invalid or expired state parameter', HttpStatus.BAD_REQUEST); + } + this.someTemporaryStore.delete(state);
♻️ Duplicate comments (2)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (1)
42-86: 🧹 Nitpick (assertive)Consider concurrency locks when cleaning up or adding codes in high-load scenarios.
While your single-instance approach might suffice for moderate usage, if you expand to multiple instances, you might need a mutex or external store to avoid race conditions when the code map is modified concurrently.- // No concurrency guard in place + // Potential improvement (future): + // Use a mutex lock or distributed lock for multi-instance scenariospackages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (1)
239-241: Ensure consistent status codes for invalid credentials [duplicate comment]
A past review noted the mismatch between returningHttpStatus.BAD_REQUESThere andHttpStatus.UNAUTHORIZEDin the refresh flow. Please unify these to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
packages/config/src/lib/environments/environment.prod.ts(1 hunks)packages/config/src/lib/environments/environment.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts(4 hunks)packages/plugins/integration-zapier/src/lib/zapier.controller.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (5)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:6-19
Timestamp: 2025-04-11T06:13:47.856Z
Learning: For the Zapier integration auth code storage, using an in-memory Map is acceptable for moderate usage volumes due to the built-in safeguards: a maximum limit of 1000 codes and an automatic cleanup mechanism for expired codes.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:93-105
Timestamp: 2025-04-11T09:03:32.978Z
Learning: For the Zapier integration auth code storage, on-demand cleanup (triggered when approaching the maximum code limit) is sufficient without the need for periodic scheduled cleanup, as determined by the implementation requirements.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:37-70
Timestamp: 2025-04-11T14:06:04.523Z
Learning: The ZapierAuthCodeService unit tests will be implemented in a future PR, as confirmed by RolandM99.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:145-165
Timestamp: 2025-04-15T13:43:30.444Z
Learning: The domain validation in ZapierAuthCodeService doesn't need to handle IP addresses, as OAuth redirects typically only require domain name validation.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:27-29
Timestamp: 2025-04-15T18:29:09.300Z
Learning: The ZapierAuthCodeService in the Zapier integration plugin is designed to be initialized only once as a singleton service in the NestJS application.
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (2)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:21-57
Timestamp: 2025-04-11T08:39:18.375Z
Learning: For Zapier integration redirect URI validation, implement domain whitelisting to prevent open redirect vulnerabilities. Check incoming URIs against a predefined list of trusted domains using URL parsing before generating auth codes.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts:319-349
Timestamp: 2025-04-15T21:47:59.835Z
Learning: The Zapier integration currently uses in-memory locks for concurrency control, with plans to implement Redis-based distributed locking in the future. This is a known limitation that should not be flagged in reviews.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (5)
packages/config/src/lib/environments/environment.prod.ts (1)
196-197: Good use of trimming for allowed domains.
This effectively handles whitespace in domain entries, reducing the risk of incomplete parsing or validation.packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (1)
125-133: Confirm that on-demand cleanup meets your SLA.
You manually clean expired codes incleanupExpiredAuthCodes. This works, but if many codes expire before the limit is reached, they remain until the next cleanup cycle. If that’s acceptable, keep as is. Otherwise, consider more frequent or event-driven purging.packages/plugins/integration-zapier/src/lib/zapier.controller.ts (2)
111-125: Differentiate between missing and invalid tokens.
Throwing aBadRequestExceptionfor missing tokens vs.UnauthorizedExceptionfor invalid tokens can help identify the problem quickly. Confirm that this distinction aligns with your intended UX.
158-166: Confirm token validation expectations.
YourvalidateTokenmethod uses eitherUnauthorizedExceptionorBadRequestExceptiondepending on flags. Ensure the consuming code knows the difference—particularly for automated integrations.packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (1)
243-248:❓ Verification inconclusive
Verify that authorization codes are single-use
It's unclear whethergetUserInfoFromAuthCode(code)invalidates the authorization code. In standard OAuth2 flows, an authorization code should be used once and then removed or marked as invalid.
🏁 Script executed:
#!/bin/bash # Check if the auth code is invalidated after retrieval ast-grep --pattern $'function getUserInfoFromAuthCode($_){\n $$$\n}'Length of output: 68
I'll run additional verification scripts to search for any code that marks the authorization code as used (e.g., calls to "invalidate", "delete", or similar) in and around the implementation of
getUserInfoFromAuthCode. For example:Based on the previous script’s output—where no output was produced—we cannot conclusively verify that the authorization code is being invalidated after retrieval. Please run these additional commands or manually inspect the implementation of
getUserInfoFromAuthCodeto ensure it adheres to OAuth2’s single-use requirement.
Action Required: Confirm Single-Use Authorization Code Handling
Please verify manually that the
getUserInfoFromAuthCode(code)method invalidates the provided authorization code immediately after a successful retrieval, as per OAuth2 best practices. If the code is not currently invalidated, update the implementation to ensure that authorization codes cannot be reused.
🏁 Script executed:
#!/bin/bash # Search for the definition of getUserInfoFromAuthCode in the source files. rg "getUserInfoFromAuthCode" -n packages/plugins/integration-zapier/src/ && \ echo "Checking for potential invalidation logic in zapier-auth-code service:" && \ rg -nE "(delete|getAuthCode|invalidate|markUsed)" packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.tsLength of output: 850
Action Required: Confirm Single-Use Authorization Code Handling
Our additional searches did not reveal any explicit logic (such as deletion, invalidation, or marking the code as used) in the implementation of
getUserInfoFromAuthCode. Please manually verify that this method invalidates the authorization code immediately upon successful retrieval to comply with OAuth2 single-use requirements.
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier.controller.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (1)
81-138: 🧹 Nitpick (assertive)Well-structured
authorizeflow.• Checks critical parameters (
state,response_type,client_id).
• ValidatesredirectUriagainstallowedDomains.
• Redirects to the login page with query parameters.One small note: in
getToken, invalid credentials result in a 400 or 401. Consider returning a consistent status code (401) for invalid credentials here as well.
♻️ Duplicate comments (2)
packages/config/src/lib/environments/environment.prod.ts (1)
196-198: 🧹 Nitpick (assertive)Simplify boolean conversion.
The ternary expression for
instanceCountcan be simplified.- instanceCount: process.env.INSTANCE_COUNT === 'true' ? true : false, + instanceCount: process.env.INSTANCE_COUNT === 'true',🧰 Tools
🪛 Biome (1.9.4)
[error] 198-198: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (1)
214-292: 🧹 Nitpick (assertive)
getTokenproperly enforces redirect consistency.Storing the redirect URI with the auth code and matching it upon token exchange is a solid security measure. However, note that mismatched credentials throw a
HttpException('Invalid client ID', HttpStatus.BAD_REQUEST)(line 243). For better clarity, consider unifying the error message and status code with theUnauthorizedExceptionapproach used in/refresh-token.- throw new HttpException('Invalid client ID', HttpStatus.BAD_REQUEST); + throw new UnauthorizedException('Invalid client credentials');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
.env.compose(1 hunks).env.demo.compose(1 hunks).env.docker(1 hunks).env.local(1 hunks).env.sample(1 hunks)packages/common/src/lib/interfaces/IZapierConfig.ts(1 hunks)packages/config/src/lib/environments/environment.prod.ts(1 hunks)packages/config/src/lib/environments/environment.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts(4 hunks)packages/plugins/integration-zapier/src/lib/zapier.controller.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (5)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:27-29
Timestamp: 2025-04-15T18:29:09.300Z
Learning: The ZapierAuthCodeService in the Zapier integration plugin is designed to be initialized only once as a singleton service in the NestJS application.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:6-19
Timestamp: 2025-04-11T06:13:47.856Z
Learning: For the Zapier integration auth code storage, using an in-memory Map is acceptable for moderate usage volumes due to the built-in safeguards: a maximum limit of 1000 codes and an automatic cleanup mechanism for expired codes.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:93-105
Timestamp: 2025-04-11T09:03:32.978Z
Learning: For the Zapier integration auth code storage, on-demand cleanup (triggered when approaching the maximum code limit) is sufficient without the need for periodic scheduled cleanup, as determined by the implementation requirements.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:37-70
Timestamp: 2025-04-11T14:06:04.523Z
Learning: The ZapierAuthCodeService unit tests will be implemented in a future PR, as confirmed by RolandM99.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:145-165
Timestamp: 2025-04-15T13:43:30.444Z
Learning: The domain validation in ZapierAuthCodeService doesn't need to handle IP addresses, as OAuth redirects typically only require domain name validation.
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (2)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:21-57
Timestamp: 2025-04-11T08:39:18.375Z
Learning: For Zapier integration redirect URI validation, implement domain whitelisting to prevent open redirect vulnerabilities. Check incoming URIs against a predefined list of trusted domains using URL parsing before generating auth codes.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts:319-349
Timestamp: 2025-04-15T21:47:59.835Z
Learning: The Zapier integration currently uses in-memory locks for concurrency control, with plans to implement Redis-based distributed locking in the future. This is a known limitation that should not be flagged in reviews.
🪛 Biome (1.9.4)
packages/config/src/lib/environments/environment.prod.ts
[error] 198-198: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
packages/config/src/lib/environments/environment.ts
[error] 198-198: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
[error] 234-234: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (27)
.env.sample (2)
156-157: Security improvement with domain allowlist in sample config.The addition of
ALLOWED_DOMAINSwith a clear comment enhances OAuth security and developer understanding. The variable is well-documented and the domain list is comprehensive for the intended integrations.
158-159: Operational variables for OAuth code management are clear.
MAX_AUTH_CODESandINSTANCE_COUNTare self-explanatory and align with best practices for managing temporary authorization codes and instance-specific behavior..env.local (2)
168-169: OAuth domain allowlist is well-documented and secure.The addition of
ALLOWED_DOMAINSwith a descriptive comment improves OAuth security and developer clarity.
170-171: Operational variables for OAuth code management are clear.
MAX_AUTH_CODESandINSTANCE_COUNTare clearly named and support robust OAuth code management..env.docker (2)
151-152: Domain restriction variable is consistent and well-formatted.
ALLOWED_DOMAINSis correctly documented and formatted, with no leading space. This ensures proper parsing and security.
153-154: Operational variables for OAuth code management are clear.
MAX_AUTH_CODESandINSTANCE_COUNTare clearly named and support robust OAuth code management..env.demo.compose (2)
154-155: OAuth domain allowlist is well-documented and secure.The addition of
ALLOWED_DOMAINSwith a descriptive comment improves OAuth security and developer clarity. No formatting issues are present.
156-157: Operational variables for OAuth code management are clear.
MAX_AUTH_CODESandINSTANCE_COUNTare clearly named and support robust OAuth code management.packages/common/src/lib/interfaces/IZapierConfig.ts (2)
11-13: Properties are well-documented.The added
allowedDomainsproperty with clear JSDoc documentation correctly enforces domain restrictions for OAuth redirects, strengthening security.
20-24: Auth code and instance management parameters are well-structured.These optional properties provide good control over auth code limits and instance behavior without forcing values to be set.
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (7)
1-46: Good implementation of auth code service with configurable limits.The service effectively manages auth codes with:
- Configurable domain allowlist
- Configurable max auth codes
- Intelligent cleanup strategy for single-instance deployments
- Clear logging of initialization parameters
I appreciate the defensive approach with special handling for multi-instance deployments.
57-97: Well-implemented auth code generation with validation.The
generateAuthCodemethod properly:
- Generates unique codes with UUID
- Sets appropriate expirations
- Validates redirect URIs against allowed domains
- Cleans up expired codes when reaching limits
- Includes detailed error logging
99-139: Thorough user info extraction with security checks.The
getUserInfoFromAuthCodemethod properly:
- Validates code existence and expiration
- Ensures redirect URI matches original request
- Provides detailed error messages with appropriate HTTP status codes
- Includes thorough debug logging
141-160: Effective cleanup implementation.The cleanup methods are well-structured with:
- On-demand cleanup in
cleanupExpiredAuthCodes- Periodic cleanup on a reasonable 5-minute interval
- Appropriate debug logging
162-194: Robust domain validation logic.The
isValidRedirectDomainmethod implements strong security with:
- Proper URL parsing with error handling
- Special handling for localhost development
- Flexible subdomain validation using domain part comparison
- Clear error logging
195-214: Useful server domain extraction utility.The
getServerDomainFromRequestmethod provides a helpful utility for dynamically determining the current domain, which could be used to enhance the allowed domains list.
216-239: Thorough instance deployment detection.The
isSingleInstanceDeploymentmethod handles multiple input formats:
- Boolean strings ('true'/'false')
- Numeric values
- Undefined values with sensible defaults
🧰 Tools
🪛 Biome (1.9.4)
[error] 234-234: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
packages/plugins/integration-zapier/src/lib/zapier.controller.ts (5)
4-19: New imports appear correct and consistent.All newly added imports and decorators (e.g.,
BadRequestException,HttpStatus,Public, etc.) are pertinent to OAuth handling and error management. No issues found with unused or missing imports.
34-110: Robust domain validation inloginSuccess.The domain whitelisting logic (lines 59–90) thoroughly checks
zapier_redirect_uriagainst localhost and configured allowed domains, preventing open redirect vulnerabilities. The fallback redirect to/dashboardalso serves as a safe default. Looks great.
114-131: Efficient token validation ingetTriggers.Invoking
this.validateToken(token, true)and handling errors withhandleZapierErrorensures unauthorized requests are properly blocked. The code is concise and aligns with best practices for endpoint security.
133-150:getActionsmirrorsgetTriggersconsistently.Both endpoints use a similar approach for token validation and error handling, keeping the API predictable and uniform. No further concerns here.
155-163: Simple, clear token checks invalidateToken.Ensures a missing or empty token triggers an exception. This method is easy to read and maintain. No issues found.
packages/plugins/integration-zapier/src/lib/zapier-authorization.controller.ts (5)
1-21: Imports and class properties are well organized.Each imported module (e.g.,
BadRequestException,ZAPIER_REDIRECT_URI,ZapierGrantType) is used in the file. DeclaringrefreshLocksandallowedDomainsat the class level is clear for concurrency control and domain validation.
22-58:onModuleInitensures essential configs are present.Reading
zapier.allowedDomainsand parsing it into an array is correct. Logging warnings when domains are absent helps highlight potential misconfigurations early. Great job guaranteeing no silent failures.
60-75: EffectivevalidateRedirectDomainhelper.This method ensures the redirect hostname belongs to the allowed list. If you require sub-subdomain handling or more nuanced checks (ports, protocols), you can extend this in the future. Minimal but sufficient for a standard domain whitelist.
148-213:callbacklogic aligns with the domain validation and code generation.The domain check, user authentication check, and final redirect to Zapier are all essential steps. Obfuscating the
codein logs is a thoughtful security measure.
293-388:refreshTokenconcurrency lock is straightforward and ephemeral.The lock prevents concurrent refresh attempts for the same
integrationId, and a short timeout cleans up the lock, preventing memory leaks. This is a practical approach. Final results are returned with a unifiedBearertoken response—no major concerns here.
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts
Outdated
Show resolved
Hide resolved
packages/plugins/integration-zapier/src/lib/zapier.controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.env.compose(1 hunks)packages/config/src/lib/environments/environment.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts(1 hunks)packages/plugins/integration-zapier/src/lib/zapier.controller.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (5)
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:27-29
Timestamp: 2025-04-15T18:29:09.300Z
Learning: The ZapierAuthCodeService in the Zapier integration plugin is designed to be initialized only once as a singleton service in the NestJS application.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:6-19
Timestamp: 2025-04-11T06:13:47.856Z
Learning: For the Zapier integration auth code storage, using an in-memory Map is acceptable for moderate usage volumes due to the built-in safeguards: a maximum limit of 1000 codes and an automatic cleanup mechanism for expired codes.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:93-105
Timestamp: 2025-04-11T09:03:32.978Z
Learning: For the Zapier integration auth code storage, on-demand cleanup (triggered when approaching the maximum code limit) is sufficient without the need for periodic scheduled cleanup, as determined by the implementation requirements.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:37-70
Timestamp: 2025-04-11T14:06:04.523Z
Learning: The ZapierAuthCodeService unit tests will be implemented in a future PR, as confirmed by RolandM99.
Learnt from: RolandM99
PR: ever-co/ever-gauzy#8901
File: packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts:145-165
Timestamp: 2025-04-15T13:43:30.444Z
Learning: The domain validation in ZapierAuthCodeService doesn't need to handle IP addresses, as OAuth redirects typically only require domain name validation.
🧬 Code Graph Analysis (2)
packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (1)
packages/desktop-core/src/lib/logger/logger.ts (1)
Logger(5-32)
packages/plugins/integration-zapier/src/lib/zapier.controller.ts (5)
packages/core/src/lib/shared/decorators/permissions.decorator.ts (1)
Permissions(5-5)packages/plugins/integration-zapier/src/lib/zapier-webhook.controller.ts (1)
Controller(20-126)packages/desktop-core/src/lib/logger/logger.ts (1)
Logger(5-32)packages/common/src/lib/decorators/public.decorator.ts (1)
Public(15-15)packages/contracts/src/lib/zapier.model.ts (1)
IZapierEndpoint(29-33)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (17)
packages/config/src/lib/environments/environment.ts (1)
196-198: Ensure consistent usage ofinstanceCountacross the codebase.You're setting
instanceCountto a boolean in this environment config. However, inZapierAuthCodeService(lines 220 onward), the code parseszapier.instanceCountas a string or number. Please verify that this boolean assignment aligns with the logic inisSingleInstanceDeployment()to avoid configuration mismatches in multi-instance environments.packages/plugins/integration-zapier/src/lib/zapier-auth-code.service.ts (8)
1-11: Imports and class setup look good.The imports for
Logger,BadRequestException,ServiceUnavailableException,uuidv4,ConfigService, and the usage ofIDare appropriate. DeclaringMAX_AUTH_CODES,AUTH_CODE_EXPIRATION_MINUTES, andALLOWED_DOMAINSas read-only class properties is a clean approach for handling these config values.
38-47: In-memory map usage is acceptable for moderate volumes.Storing auth codes in a
Mapis straightforward for single-instance deployments. Just be sure the environment constraints (MAX_AUTH_CODES) and cleanup strategy are well-communicated to prevent accidental high-memory usage if usage scales.
99-139: Good practice: delete the code once it's used or expires.Your logic properly removes the auth code once it's retrieved or found invalid/expired. This ensures an efficient cleanup, preventing accumulation of stale entries. Great job!
141-149: Efficient on-demand cleanup.The explicit iteration to remove expired codes is concise. If usage surges, consider concurrency (mutex) or distributed storage, but for a single-instance scenario, this is fine.
151-160: Periodic cleanup approach is reasonable.You enable periodic cleanup only in single-instance deployments (via
isSingleInstanceDeployment()). This design helps avoid overlapping timers in multi-instance scenarios. Nice logic separation!
162-194: Flexible subdomain validation with reversed-parts matching.This approach (comparing segments of
allowedDomainand the actual domain in reverse) effectively supports nested subdomains. Keep an eye on possible domain corner cases (e.g., TLDs with multiple dots like.co.uk), but otherwise it’s a solid method.
195-215: Method for retrieving server domain from the request is helpful.Providing an alternative route for dynamic domain retrieval adds flexibility. If domain checks rely on it, ensure you handle cases where
hostmight be undefined or empty (though you do check if request/headers exist).
217-240: Robust multi-instance detection.You're checking multiple cases (
'true','false', numeric parsing) before defaulting or throwing an error. This helps avoid ambiguous runtime states. Good practice for environment-driven config.packages/plugins/integration-zapier/src/lib/zapier.controller.ts (7)
4-14: Imports and decorators are properly structured.Using
BadRequestException,HttpStatus, andPublicis correct. Marking@Public()onloginSuccessensures it bypasses guards for Zapier redirects. The approach is consistent with the rest of the code.
29-38: Constructor includes config references for Zapier.The JSDoc clarifies which environment variables are essential for Zapier flows. This is beneficial for future maintainers who need to confirm correct environment setup.
40-118: Comprehensive validation ofzapier_redirect_uri.You thoroughly check localhost scenarios, subdomain matching with
authorizedDomains, and handle URL parsing errors. Logging unauthorized attempts is a good practice. If you foresee TLD or unusual subdomain structures, consider additional logging or an optional fallback environment variable. Otherwise this is solid.
120-137: Uniform token validation for triggers.You use
validateToken(token, true)to guard thegetTriggersendpoint. This pattern ensures consistent error handling. ThehandleZapierErrormethod further refines your error flow. Great approach.
139-156: Best practice carried over to actions.
getActionsalso callsvalidateTokenand defers error handling tohandleZapierError. This consistent approach avoids repetitive implementation details. Nice to see the uniform design.
161-169: Method-level tunable error type is a neat idea.
validateToken(token, isThrowUnauthorizedOnMissingToken)is flexible. For some endpoints missing a token can be a bad request, for others it can be unauthorized. This explicit approach is clean and easy to read.
174-191: Clear re-throwing strategy inhandleZapierError.You prioritize known exceptions (Unauthorized, BadRequest, NotFound) and fallback with
InternalServerError. This pattern ensures your API surface is predictable, while preserving the original error message for troubleshooting..env.compose (1)
152-155: Environment variables are correctly added and formatted.No issues found—these variables are well-documented and properly formatted.
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.