feat:workflow node use work thread #123
Conversation
WalkthroughThis pull request introduces enhancements to the workflow module's handling of worker threads and resource management. The changes span multiple files in the Changes
Sequence DiagramsequenceDiagram
participant Client
participant WorkflowNode
participant PluginWorkflow
participant WorkerThread
Client->>WorkflowNode: Configure useWorker
WorkflowNode->>PluginWorkflow: Create/Update Request
alt useWorker is true
PluginWorkflow->>WorkerThread: Process via Worker
else
PluginWorkflow->>PluginWorkflow: Process Directly
end
WorkerThread-->>PluginWorkflow: Return Result
PluginWorkflow-->>Client: Return Final Result
Possibly Related PRs
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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 (
|
WalkthroughThis pull request introduces a new feature to the workflow module by adding support for using worker threads. It includes modifications to handle resources such as URLs, JSON objects, and base64 data, and integrates these changes into the workflow's server-side logic. The update aims to enhance the efficiency and functionality of workflow operations by leveraging worker threads. Changes
🪧 TipsFor further assistance, please describe your question in the comments and @petercat-assistant to start a conversation with me. |
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| contentType, | ||
| }); | ||
| } else { | ||
| throw new Error('Invalid data URL format'); |
There was a problem hiding this comment.
The error message 'Invalid data URL format' should be more descriptive to aid debugging. Consider including the problematic URL or additional context.
| contentType: response.headers['content-type'], | ||
| }); | ||
| } | ||
| const uploadResponse = await axios({ |
There was a problem hiding this comment.
Ensure that the axios request to upload attachments handles potential errors, such as network issues or server errors, to prevent unhandled promise rejections.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/module-workflow/src/server/instructions/UpdateInstruction.ts (1)
22-29: Consider transaction error handling.
When pulling a transaction fromthis.workflow.useDataSourceTransaction, it may be beneficial to catch and handle errors (e.g., if the data source is unavailable) or confirm it is not null to avoid unexpected behavior.packages/module-workflow/src/server/instructions/CreateInstruction.ts (2)
29-30: Transaction vs. Worker usage.
Similar toUpdateInstruction, confirm that skipping worker usage whentransactionis present aligns with design requirements. Some workflows may need both a transaction and worker threads.
35-36: Avoid accidental mutation of processor context.
context.stackis extended by merging the currentprocessor.execution.id. Confirm that subsequent instructions won't unintentionally see updated context from earlier instructions in an unexpected manner.packages/module-workflow/src/server/Plugin.ts (3)
7-8: Check external dependencies.
axiosandFormDatacan be large dependencies. Confirm these are indeed needed at runtime and consider factoring them into a separate helper if usage grows.
666-701: Optimize attachment field detection.
You're looping over fields in a collection to handle attachments. Re-check potential performance impacts on large sets or repeated calls. Consider caching or short-circuiting if no attachments exist.
722-740: Confirm return type for batch updates.
return result.length ?? result;can produce a numeric array length or the full result object. Ensure callers ofworkerWorkflowUpdateexpect the correct data shape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/module-workflow/src/client/nodes/create.tsx(1 hunks)packages/module-workflow/src/server/Plugin.ts(2 hunks)packages/module-workflow/src/server/instructions/CreateInstruction.ts(2 hunks)packages/module-workflow/src/server/instructions/UpdateInstruction.ts(2 hunks)
🔇 Additional comments (12)
packages/module-workflow/src/server/instructions/UpdateInstruction.ts (3)
7-7: Check import path.
Make sure '..' correctly resolves to the location of PluginWorkflowServer.
31-55: Validate synchronous vs. worker-based approach.
Using node?.config?.useWorker && !transaction && app.worker.available to determine if the worker is invoked is a neat approach. However, consider how partial updates or transaction rollbacks are handled if a worker call encounters a failure. Ensure all relevant states remain consistent if an operation in the worker context fails unexpectedly.
59-59: Return structure clarity.
Returning { result, status: JOB_STATUS.RESOLVED } is clear. Just ensure any consumer of UpdateInstruction accounts for this JSON structure.
packages/module-workflow/src/server/instructions/CreateInstruction.ts (5)
11-13: Imports alignment.
PluginWorkflow and PluginWorkflowServer are imported. Verify these references point to the expected modules and won't cause circular dependencies.
23-24: Clarify user identity handling.
Storing userId from the scope and embedding it in a signed token is a valid approach. Just confirm that subsequent checks fully respect role-based or permissions-based logic if needed.
26-27: Remind to handle origin fallback scenario.
origin = Gateway.getInstance().runAtLoop might be undefined in uncommon scenarios. If there's a chance runAtLoop is unavailable, add checks to avoid runtime errors.
32-32: Avoid direct repository usage if plugin usage is expected.
Confirm that referencing .repository is still desired, since the new approach uses plugin-based updates.
39-63: Worker logic clarity.
Your logic to use the worker if available, otherwise default to plugin.workerWorkflowCreate, is consistent with the rest of the codebase. Confirm that aggregate or bulk creation tasks remain performant under worker-based creation.
packages/module-workflow/src/server/Plugin.ts (3)
5-5: Validate utils import usage.
{ Registry, uid } is imported. Ensure all references to uid in this file are correct and do not conflict with any local naming.
591-664: Guard resource anomalies.
While handling multiple resource formats (base64, JSON, external URLs) is robust, ensure each branch checks for empty, invalid, or malicious data. For instance, an empty data URL could cause an error.
703-720: workerWorkflowCreate method clarity.
This method processes attachments, then calls collection.repository.create. If it's truly used in a worker context, confirm concurrency controls on the repository.
packages/module-workflow/src/client/nodes/create.tsx (1)
49-58: New field 'useWorker'.
Great addition for controlling worker usage. Ensure the front-end usage of 'useWorker' aligns with server logic and handle potential toggling gracefully in the UI.
| import axios, { AxiosRequestConfig } from 'axios'; | ||
| import FormData from 'form-data'; | ||
| import LRUCache from 'lru-cache'; | ||
| import mime from 'mime-types'; |
There was a problem hiding this comment.
Remember to handle unknown MIME types.
mime-types may return false. Handle the scenario where mime.extension(contentType) is null or undefined to avoid constructing invalid filenames.
- const ext = mime.extension(contentType);
+ const ext = mime.extension(contentType) || 'bin';Committable suggestion skipped: line range outside the PR's diff.
| isJSON(str) { | ||
| try { | ||
| return JSON.parse(str); | ||
| } catch (e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check JSON parse logic.
isJSON(str) returns the parsed object on success or false on failure. The name suggests a boolean return, but actually returns parsed JSON or false. This might be confusing. Either rename it or return only booleans.
- isJSON(str) {
- try {
- return JSON.parse(str);
- } catch (e) {
- return false;
- }
- }
+ isJSON(str) {
+ try {
+ JSON.parse(str);
+ return true;
+ } catch (e) {
+ return false;
+ }
+ }📝 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.
| isJSON(str) { | |
| try { | |
| return JSON.parse(str); | |
| } catch (e) { | |
| return false; | |
| } | |
| } | |
| isJSON(str) { | |
| try { | |
| JSON.parse(str); | |
| return true; | |
| } catch (e) { | |
| return false; | |
| } | |
| } |
baizixv
changed the title
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates