New Components - ignisign #14577
New Components - ignisign #14577
Conversation
luancazarine
added
the
ai-assisted
|
Warning Rate limit exceeded@luancazarine has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces several new modules and functionalities for the IgniSign application, including actions for creating signature requests, signers, and retrieving signature proofs. It also adds utility functions and constants for managing language options. Significant modifications were made to the application’s main file to enhance property definitions and API interactions. Additionally, a new source module for emitting events related to signature proofs was created, along with a test event module to facilitate development and verification. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Sources - New Signature Proof (Instant) Actions - Create Signer - Create Signature Request - Get Signature Proof
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (11)
components/ignisign/common/constants.mjs (1)
1-14: Consider enhancing the language options data structure.While the current array implementation is simple, consider using an object structure that includes additional metadata for better maintainability and extensibility:
-export const LANGUAGE_OPTIONS = [ - "EN", - "FR", - "DE", - "ES", - "IT", - "PT", - "NL", - "PL", - "JA", - "KO", - "AR", - "HE", -]; +export const LANGUAGE_OPTIONS = { + EN: { name: 'English', rtl: false }, + FR: { name: 'French', rtl: false }, + DE: { name: 'German', rtl: false }, + ES: { name: 'Spanish', rtl: false }, + IT: { name: 'Italian', rtl: false }, + PT: { name: 'Portuguese', rtl: false }, + NL: { name: 'Dutch', rtl: false }, + PL: { name: 'Polish', rtl: false }, + JA: { name: 'Japanese', rtl: false }, + KO: { name: 'Korean', rtl: false }, + AR: { name: 'Arabic', rtl: true }, + HE: { name: 'Hebrew', rtl: true } +}; + +// Helper to get array of codes for backward compatibility +export const LANGUAGE_CODES = Object.keys(LANGUAGE_OPTIONS);components/ignisign/common/utils.mjs (1)
1-36: Consider architectural improvements for production readiness.To enhance maintainability and robustness, consider these architectural improvements:
Create a centralized error handling strategy:
- Define custom error types for different failure scenarios
- Implement consistent error reporting
Add logging integration:
- Use a proper logging framework instead of console.warn
- Include request IDs for better traceability
Implement configuration management:
- Move hardcoded values (like '/tmp' and MAX_DEPTH) to configuration
- Allow environment-specific settings
Would you like me to provide example implementations for any of these improvements?
components/ignisign/sources/new-signature-proof-instant/test-event.mjs (2)
1-23: Add JSDoc comments to document the test event structure.Since this is a test event file that other developers might reference when implementing webhook handlers, it would be helpful to add documentation explaining that this is sample data and describing the expected payload structure.
Add this documentation at the top of the file:
+/** + * Sample webhook payload for testing the Ignisign signature proof event handler. + * This structure matches the payload sent by Ignisign when a new signature proof is generated. + * @see https://documentation.ignisign.io/webhooks/signature-proof + */ export default {🧰 Tools
🪛 Gitleaks
15-15: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
2-3: Consider using a more obvious test app ID.The current app ID looks like a real UUID. To prevent confusion, consider using a more obvious test identifier.
- "appId": "appId_e5d6bcfb-e9f4-4858-8027-b65e1a170bd7", + "appId": "test-app-id", "appEnv": "DEVELOPMENT",Also applies to: 8-9
components/ignisign/actions/get-signature-proof/get-signature-proof.mjs (1)
31-46: Consider using object storage for better scalability.The current implementation stores proof files in the local filesystem (
/tmp). In a distributed environment, this approach may not scale well and could cause issues with file accessibility across different instances.Consider using object storage (like AWS S3, Google Cloud Storage, etc.) instead of local filesystem for better scalability and reliability.
components/ignisign/sources/new-signature-proof-instant/new-signature-proof-instant.mjs (2)
7-7: Consider plain text for the description.The markdown link in the description might not render properly in all contexts. Consider using plain text with the URL appended.
- description: "Emit new event when a signature proof is generated. [See the documentation](https://ignisign.io/docs/webhooks/signatureproof)", + description: "Emit new event when a signature proof is generated. See the documentation: https://ignisign.io/docs/webhooks/signatureproof",
11-18: Add type validation for HTTP response.Consider adding response type validation to ensure webhook payload consistency.
http: { type: "$.interface.http", customResponse: true, + responseType: "json", + responseData: { + status: "success", + message: "Webhook received" + } },components/ignisign/actions/create-signature-request/create-signature-request.mjs (3)
39-39: Correct typo in 'Document External Id' descriptionThere's a typo in the description for
documentExternalId: "Ignisign's system do not interprete it." It should be "Ignisign's system does not interpret it."Apply this change:
description: "An optional external identifier that can be used to reference the document from external systems. It's a free text. Ignisign's system do not interprete it.", + description: "An optional external identifier that can be used to reference the document from external systems. It's free text. Ignisign's system does not interpret it.",
66-71: Validate format of 'Expiration Date'Ensure that the
expirationDateproperty receives a properly formatted date string. Consider adding validation to confirm that the date is in the expected format to prevent errors during API requests.You might add a validation step like:
import { isValid, parseISO } from "date-fns"; // Inside your run method or before sending the data: if (this.expirationDateIsActivated && this.expirationDate) { const date = parseISO(this.expirationDate); if (!isValid(date)) { throw new ConfigurationError("Invalid expiration date format. Please provide a valid ISO 8601 date string."); } }
59-65: Set default value for 'Expiration Date Is Activated'Consider setting a default value for the
expirationDateIsActivatedproperty tofalseto ensure predictable behavior when the property is not provided.Apply this change:
expirationDateIsActivated: { type: "boolean", label: "Expiration Date Is Activated", description: "Indicates whether the expiration date is activated.", reloadProps: true, optional: true, + default: false, },components/ignisign/ignisign.app.mjs (1)
11-96: Refactorasync optionsmethods to reduce code duplicationThe
async optionsmethods withinpropDefinitionsshare similar logic for fetching data and mapping it to options. Consider refactoring this repetitive code into a reusable helper function to improve maintainability and adhere to the DRY principle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
components/ignisign/actions/create-signature-request/create-signature-request.mjs(1 hunks)components/ignisign/actions/create-signer/create-signer.mjs(1 hunks)components/ignisign/actions/get-signature-proof/get-signature-proof.mjs(1 hunks)components/ignisign/common/constants.mjs(1 hunks)components/ignisign/common/utils.mjs(1 hunks)components/ignisign/ignisign.app.mjs(1 hunks)components/ignisign/package.json(2 hunks)components/ignisign/sources/new-signature-proof-instant/new-signature-proof-instant.mjs(1 hunks)components/ignisign/sources/new-signature-proof-instant/test-event.mjs(1 hunks)
🧰 Additional context used
🪛 Gitleaks
components/ignisign/sources/new-signature-proof-instant/test-event.mjs
15-15: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (11)
components/ignisign/common/constants.mjs (1)
1-14: Verify language codes against Ignisign's API requirements.
Please verify that:
- The uppercase format matches Ignisign's API expectations (ISO 639-1 typically uses lowercase)
- This list covers all languages supported by Ignisign
- No essential languages are missing (e.g., Chinese, Russian)
components/ignisign/package.json (2)
3-3: LGTM on version number
The version number follows semantic versioning and is appropriate for an initial release.
12-14: LGTM on publish configuration
The public access configuration is appropriate for a Pipedream component.
components/ignisign/sources/new-signature-proof-instant/test-event.mjs (2)
1-6: LGTM! Event metadata structure matches Ignisign's webhook format.
The basic event metadata structure correctly implements the required fields for Ignisign's webhook payload.
15-16: Verify the token and URL structure matches Ignisign's format.
The current implementation includes tokens and URLs that appear to follow Ignisign's format. Let's verify this against their documentation.
Also applies to: 19-20, 22-22
✅ Verification successful
Based on the search results and the component's source code, I can now generate a final response:
The token and URL structure in the test event matches Ignisign's implementation
The token and URL patterns are consistent with Ignisign's implementation as evidenced by:
- The URLs follow Ignisign's official domain pattern
sign.ignisign.io - The URL structure follows the documented format:
/signature-requests/{requestId}/proofs?t={token} - The tokens are consistently formatted base64-like strings of appropriate length
- The implementation is part of the official Ignisign component that integrates with their API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any other test files or documentation that might confirm the token format
rg -g '!node_modules' -g '*.{md,txt}' -i 'ignisign.*token.*format'
Length of output: 68
Script:
#!/bin/bash
# Let's search for test files and sample data related to Ignisign tokens
rg -g '!node_modules' -g '*.{js,mjs,json}' -B2 -A2 "ignisign.*token"
# Also search for any URL patterns in test files
rg -g '!node_modules' -g '*test*.{js,mjs,json}' "sign\.ignisign\.io"
# Look for any documentation or configuration files mentioning Ignisign
fd -g 'README*' -g '*.md' -x cat {} | rg -i 'ignisign'
Length of output: 725
Script:
#!/bin/bash
# Let's check the Ignisign component's source code and documentation
fd -t f -e md -e js -e mjs -e json . components/ignisign/ -x cat {}
# Let's also look for any API-related configurations or schemas
rg -g '!node_modules' -g '*.{json,js,mjs}' -B2 -A2 'ignisign.*api'
# Search for any validation patterns or schemas
ast-grep --pattern 'const $_ = {
$$$
token: $_,
$$$
}'
Length of output: 22116
🧰 Tools
🪛 Gitleaks
15-15: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
components/ignisign/actions/get-signature-proof/get-signature-proof.mjs (2)
1-4: LGTM: Imports are appropriate for the functionality.
The imports correctly include the necessary Node.js built-in modules for file streaming operations and the Ignisign app module.
6-30: LGTM: Action configuration aligns with requirements.
The action metadata and property definitions are well-structured and match the requirements from issue #14570. The dependency between documentId and signatureRequestId is properly implemented.
components/ignisign/sources/new-signature-proof-instant/new-signature-proof-instant.mjs (1)
4-54: Verify webhook implementation against requirements.
The implementation appears to meet the PR objectives for emitting signature proof events. However, let's verify the webhook event types are properly configured.
✅ Verification successful
After analyzing the codebase, I can now provide a final response regarding the webhook implementation verification:
Webhook implementation correctly handles signature proof events
The implementation is properly configured for signature proof events:
- The webhook is created with the correct endpoint during activation
- All events are initially disabled via
disableWebhookEvents - The deduplication is properly set up using unique IDs
- The sample event demonstrates correct handling of signature proof payloads
- The webhook cleanup is handled in the deactivate hook
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify webhook configuration and event types
# Test: Search for webhook event type configurations
rg -A 5 "createWebhook|disableWebhookEvents|enableWebhookEvents" --type js
# Test: Search for signature proof related constants or configurations
rg "SIGNATURE_PROOF|signatureProof|signature_proof" --type js
Length of output: 140
Script:
#!/bin/bash
# Let's try a broader search to understand webhook and event handling
# Search for webhook-related code
rg -l "webhook" --type js
# Search for event types or configurations
rg -l "EVENT_TYPES|eventTypes|EVENTS" --type js
# Search in the Ignisign app directory
fd . components/ignisign -t f -e js -e mjs | xargs cat
Length of output: 18718
components/ignisign/actions/create-signer/create-signer.mjs (1)
1-8: LGTM! Well-documented action metadata.
The action metadata is well-structured and includes a helpful documentation link.
components/ignisign/ignisign.app.mjs (2)
63-63: Verify pagination logic in signatureRequestId options
You are incrementing the page parameter with page: page + 1, whereas in other options methods, page is passed directly. Ensure that the pagination logic is consistent across methods to prevent off-by-one errors.
230-230: Ensure consistent use of envs parameter in API requests
In this method, envs is set to an empty string, altering the base URL by excluding the environment-specific path. Verify that this usage is intentional and consistent with other API calls, as inconsistent usage may lead to incorrect endpoints.
components/ignisign/actions/create-signature-request/create-signature-request.mjs
Outdated
Show resolved
Hide resolved
components/ignisign/actions/create-signature-request/create-signature-request.mjs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hi @luancazarine I've just left a couple of minor observations other than that lgtm! Ready for QA!
components/ignisign/actions/create-signature-request/create-signature-request.mjs
Show resolved
Hide resolved
…gnature-proof-instant.mjs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
components/ignisign/actions/create-signature-request/create-signature-request.mjs (1)
42-46: Enhance file prop documentation with persistence warning.The file prop description should include a warning about the temporary nature of files in
/tmpdirectory. Files in/tmpare not persisted across executions.Apply this diff to improve the documentation:
file: { type: "string", label: "Document File", - description: "The file to be uploaded, please provide a file from `/tmp`. To upload a file to `/tmp` folder, please follow the doc [here](https://pipedream.com/docs/code/nodejs/working-with-files/#writing-a-file-to-tmp)", + description: "The file to be uploaded, please provide a file from `/tmp`. Note: Files in `/tmp` are temporary and not persisted across executions. To upload a file to `/tmp` folder, please follow the doc [here](https://pipedream.com/docs/code/nodejs/working-with-files/#writing-a-file-to-tmp)", },components/ignisign/ignisign.app.mjs (1)
274-293: Consider making webhook topics configurableThe
disableWebhookEventsmethod hardcodes all topics to be disabled. Consider making it more flexible by accepting a topics parameter.Apply this diff to make the method more reusable:
- disableWebhookEvents(webhookId) { + disableWebhookEvents(webhookId, topics = { + "ALL": true, + "APP": true, + "SIGNATURE": true, + "SIGNATURE_REQUEST": true, + "SIGNER": true, + "SIGNATURE_PROFILE": true, + "SIGNATURE_SESSION": true, + "SIGNATURE_SIGNER_IMAGE": true, + "ID_PROOFING": true, + "SIGNER_AUTH": true, + }) { return this._makeRequest({ method: "POST", path: `/webhooks/${webhookId}/disabled-events`, envs: "", data: { - topics: { - "ALL": true, - "APP": true, - "SIGNATURE": true, - "SIGNATURE_REQUEST": true, - "SIGNER": true, - "SIGNATURE_PROFILE": true, - "SIGNATURE_SESSION": true, - "SIGNATURE_SIGNER_IMAGE": true, - "ID_PROOFING": true, - "SIGNER_AUTH": true, - }, + topics, }, }); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
components/ignisign/actions/create-signature-request/create-signature-request.mjs(1 hunks)components/ignisign/actions/create-signer/create-signer.mjs(1 hunks)components/ignisign/ignisign.app.mjs(1 hunks)components/ignisign/sources/new-signature-proof-instant/new-signature-proof-instant.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/ignisign/actions/create-signer/create-signer.mjs
- components/ignisign/sources/new-signature-proof-instant/new-signature-proof-instant.mjs
🔇 Additional comments (6)
components/ignisign/actions/create-signature-request/create-signature-request.mjs (3)
1-9: LGTM! Well-structured imports.
The imports are well-organized with clear separation between standard libraries and custom utilities.
10-15: LGTM! Well-defined action metadata.
The action metadata is properly defined with appropriate versioning and documentation links.
80-83: LGTM! Correct implementation of dynamic prop visibility.
The additionalProps method correctly handles the visibility of the expirationDate field based on the expirationDateIsActivated flag.
components/ignisign/ignisign.app.mjs (3)
1-6: LGTM!
The app configuration and imports are correctly set up.
67-73: Verify the signature request status filter requirement
The options method filters for "COMPLETED" status only. Please verify if this is the intended behavior as it might prevent users from selecting in-progress signature requests.
#!/bin/bash
# Description: Check if there are any API endpoints or components that need access to non-completed signature requests
# Search for signature request status references
rg -l "status.*COMPLETED" --type js
# Search for signature request selection in actions
ast-grep --pattern 'signatureRequestId: {
type: "string",
$$$
}'207-216:
Inconsistent environment parameter usage
Several methods override the default environment parameter with an empty string. This inconsistency could indicate a potential issue with environment handling.
Consider one of these approaches:
- Document why these specific endpoints don't require environment context
- Standardize the environment parameter usage across all methods
Also applies to: 217-222, 223-232, 243-252, 253-259, 267-273, 274-293
| expirationDateIsActivated: this.expirationDateIsActivated, | ||
| expirationDate: this.expirationDate, |
There was a problem hiding this comment.
Add validation for expirationDate format.
When expirationDateIsActivated is true, the expirationDate should be validated to ensure it's in a proper date format before making the API call.
Apply this diff to add date validation:
await this.ignisign.updateSignatureRequest({
signatureRequestId,
data: {
title: this.title,
description: this.description,
expirationDateIsActivated: this.expirationDateIsActivated,
- expirationDate: this.expirationDate,
+ expirationDate: this.expirationDateIsActivated
+ ? (new Date(this.expirationDate).toString() === "Invalid Date"
+ ? throw new ConfigurationError("Invalid expiration date format")
+ : this.expirationDate)
+ : undefined,
language: this.language,
documentIds: [documentId],
signerIds: parseObject(this.signerIds),
},
});Committable suggestion skipped: line range outside the PR's diff.
| async options({ page }) { | ||
| const { signatureRequests } = await this.listSignatureRequests({ | ||
| params: { | ||
| page: page + 1, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Inconsistent page handling in options method
The page parameter is incremented by 1 (page: page + 1), while other options methods use the page parameter directly. This inconsistency could lead to pagination issues.
Apply this diff to maintain consistency:
async options({ page }) {
const { signatureRequests } = await this.listSignatureRequests({
params: {
- page: page + 1,
+ page,
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async options({ page }) { | |
| const { signatureRequests } = await this.listSignatureRequests({ | |
| params: { | |
| page: page + 1, | |
| }, | |
| }); | |
| async options({ page }) { | |
| const { signatureRequests } = await this.listSignatureRequests({ | |
| params: { | |
| page, | |
| }, | |
| }); |
Resolves #14570.
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Version Update