feat: exec mediawiki workflow on module changes #479
Conversation
WalkthroughThis update adds special handling for the "Hivemind" module on the "MediaWiki" platform within the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ModuleService
participant PlatformModel
participant TemporalMediaWikiService
Client->>ModuleService: updateModule(moduleName="Hivemind", platform="MediaWiki", ...)
ModuleService->>PlatformModel: findById(platformId)
PlatformModel-->>ModuleService: platformDoc
alt platform is activated and no workflowId
ModuleService->>TemporalMediaWikiService: executeWorkflow(platformId)
TemporalMediaWikiService-->>ModuleService: workflowHandle (with workflowId)
ModuleService->>PlatformModel: save workflowId in platform metadata
end
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 4
🧹 Nitpick comments (1)
src/services/temporal/mediaWiki.service.ts (1)
42-42: Consider adding type annotations for better type safety.While the singleton pattern is appropriate, consider adding explicit type annotations for the exported instance to improve type safety and IDE support.
-export default new TemporalMediaWikiService(); +const temporalMediaWikiService: TemporalMediaWikiService = new TemporalMediaWikiService(); +export default temporalMediaWikiService;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/services/module.service.ts(3 hunks)src/services/temporal/mediaWiki.service.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/services/module.service.ts (2)
Learnt from: Behzad-rabiei
PR: TogetherCrew/api#350
File: src/services/module.service.ts:55-71
Timestamp: 2024-07-27T19:38:44.885Z
Learning: Error handling for the `updateModule` function in `src/services/module.service.ts` is managed by the parent function, as indicated by user Behzad-rabiei.
Learnt from: Behzad-rabiei
PR: TogetherCrew/api#350
File: src/services/module.service.ts:55-71
Timestamp: 2024-10-09T08:57:20.287Z
Learning: Error handling for the `updateModule` function in `src/services/module.service.ts` is managed by the parent function, as indicated by user Behzad-rabiei.
🔇 Additional comments (1)
src/services/module.service.ts (1)
97-99: LGTM! Clean integration with existing module update logic.The conditional check follows the established pattern and integrates well with the existing codebase structure.
| public async terminateWorkflow(workflowId: string): Promise<void> { | ||
| const client: Client = await this.getClient(); | ||
| const handle = client.workflow.getHandle(workflowId); | ||
| const description = await handle.describe(); | ||
| if (description.status.name !== 'TERMINATED' && description.status.name !== 'COMPLETED') { | ||
| await handle.terminate('Terminated due to schedule deletion'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for workflow handle retrieval.
The terminateWorkflow method doesn't handle potential errors when getting the workflow handle or describing its status. If the workflow doesn't exist or is inaccessible, this could throw an unhandled exception.
public async terminateWorkflow(workflowId: string): Promise<void> {
const client: Client = await this.getClient();
- const handle = client.workflow.getHandle(workflowId);
- const description = await handle.describe();
- if (description.status.name !== 'TERMINATED' && description.status.name !== 'COMPLETED') {
- await handle.terminate('Terminated due to schedule deletion');
+ try {
+ const handle = client.workflow.getHandle(workflowId);
+ const description = await handle.describe();
+ if (description.status.name !== 'TERMINATED' && description.status.name !== 'COMPLETED') {
+ await handle.terminate('Terminated due to schedule deletion');
+ }
+ } catch (error) {
+ logger.error(`Failed to terminate workflow ${workflowId}: ${(error as Error).message}`);
+ throw new Error(`Failed to terminate workflow ${workflowId}: ${(error as Error).message}`);
}
}📝 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.
| public async terminateWorkflow(workflowId: string): Promise<void> { | |
| const client: Client = await this.getClient(); | |
| const handle = client.workflow.getHandle(workflowId); | |
| const description = await handle.describe(); | |
| if (description.status.name !== 'TERMINATED' && description.status.name !== 'COMPLETED') { | |
| await handle.terminate('Terminated due to schedule deletion'); | |
| } | |
| } | |
| public async terminateWorkflow(workflowId: string): Promise<void> { | |
| const client: Client = await this.getClient(); | |
| try { | |
| const handle = client.workflow.getHandle(workflowId); | |
| const description = await handle.describe(); | |
| if (description.status.name !== 'TERMINATED' && description.status.name !== 'COMPLETED') { | |
| await handle.terminate('Terminated due to schedule deletion'); | |
| } | |
| } catch (error) { | |
| logger.error(`Failed to terminate workflow ${workflowId}: ${(error as Error).message}`); | |
| throw new Error(`Failed to terminate workflow ${workflowId}: ${(error as Error).message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/services/temporal/mediaWiki.service.ts around lines 32 to 39, the
terminateWorkflow method lacks error handling for failures when retrieving the
workflow handle or describing its status. Wrap the calls to getHandle and
describe in a try-catch block to catch any exceptions, and handle errors
gracefully by logging or rethrowing with a clear message to prevent unhandled
exceptions if the workflow is missing or inaccessible.
| public async executeWorkflow(platformId: Types.ObjectId) { | ||
| const client: Client = await this.getClient(); | ||
| const payload = { | ||
| platform_id: platformId, | ||
| }; | ||
| try { | ||
| const workflowHandle = await client.workflow.execute('MediaWikiETLWorkflow', { | ||
| taskQueue: queues.TEMPORAL_QUEUE_PYTHON_HEAVY, | ||
| args: [payload], | ||
| workflowId: `mediawiki/${platformId}/${uuidv4()}`, | ||
| }); | ||
| logger.info(`Started MediaWiki workflow with ID: ${workflowHandle}`); | ||
| return workflowHandle; | ||
| } catch (error) { | ||
| logger.error(`Failed to trigger MediaWiki workflow: ${(error as Error).message}`); | ||
| throw new Error(`Failed to trigger MediaWiki workflow: ${(error as Error).message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the return type and value of executeWorkflow method.
The method returns workflowHandle from client.workflow.execute(), but this value is later stored as workflowId in the platform metadata. Temporal's workflow.execute() typically returns a WorkflowHandle object, not a string ID.
🏁 Script executed:
#!/bin/bash
# Description: Check the return type of Temporal client.workflow.execute() method
# Expected: Confirm if it returns a WorkflowHandle object or a string ID
# Search for other usages of client.workflow.execute in the codebase
rg -A 10 "client\.workflow\.execute" --type tsLength of output: 2072
🌐 Web query:
In Temporal TypeScript SDK, what is the return type of client.workflow.execute()?
💡 Result:
In the Temporal TypeScript SDK, the client.workflow.execute() method returns a Promise that resolves to the return value of the workflow's main function.
To elaborate, when you call execute() on a workflow, it starts the workflow execution and returns a promise. Once the workflow completes, the promise resolves with the workflow's result, which is the value returned by the workflow's main function.
For example, if your workflow function returns a string, then the return type of execute() will be Promise<string>. If it returns an object of some type, the promise will resolve to that object type.
Here is a simplified example:
const result: Promise<string> = client.workflow.execute(myWorkflow, {
taskQueue: "my-task-queue",
args: ["some input"],
});If you want, I can provide more detailed code examples or official documentation references. Would you like that?
Fix executeWorkflow return type misuse: execute returns the workflow’s result, not a handle
In Temporal’s TypeScript SDK, client.workflow.execute() returns a Promise that resolves to whatever your workflow’s main function returns (e.g., a string or object), not a WorkflowHandle. Yet here you name the returned value workflowHandle, log it as an ID, and return it expecting to use it as a handle elsewhere (e.g. in terminateWorkflow).
You should either:
- Switch to
client.workflow.start()to obtain a trueWorkflowHandle, then log and return its.workflowId, or - If you really want to wait for and return the workflow’s result, rename the variable to something like
result/workflowResult, adjust your logging, and update your return type to match.
Locations to update:
- src/services/temporal/mediaWiki.service.ts (lines 13–30)
- Likewise in src/services/temporal/hivemind.service.ts for consistency
Example using start():
- const workflowHandle = await client.workflow.execute('MediaWikiETLWorkflow', { … });
- logger.info(`Started MediaWiki workflow with ID: ${workflowHandle}`);
- return workflowHandle;
+ const handle = await client.workflow.start('MediaWikiETLWorkflow', { … });
+ logger.info(`Started MediaWiki workflow with ID: ${handle.workflowId}`);
+ return handle;🤖 Prompt for AI Agents
In src/services/temporal/mediaWiki.service.ts lines 13 to 30, the
executeWorkflow method incorrectly treats the result of
client.workflow.execute() as a WorkflowHandle, but it actually returns the
workflow's result. To fix this, replace client.workflow.execute() with
client.workflow.start() to get a true WorkflowHandle, then log and return its
workflowId. Update variable names accordingly to reflect that it is a handle,
not a result. Also ensure the return type matches the handle or workflowId.
Apply similar changes in src/services/temporal/hivemind.service.ts for
consistency.
| const handleHivemindMediaWikiCase = async (platform: any) => { | ||
| const platformDoc = await platformService.getPlatformById(platform.platform); | ||
|
|
||
| if (!platformDoc) return; | ||
|
|
||
| const isActivated = platform.metadata?.activated; | ||
| const existingWorkflowId = platformDoc.get('metadata.workflowId'); | ||
|
|
||
| if (isActivated === true) { | ||
| if (!existingWorkflowId) { | ||
| const workflowId = await temporalMediaWiki.executeWorkflow(platform.platform); | ||
| platformDoc.set('metadata.workflowId', workflowId); | ||
| await platformDoc.save(); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle the deactivation scenario for consistency.
The handleHivemindMediaWikiCase function only handles the activation case but doesn't handle deactivation (when isActivated === false). This is inconsistent with the existing handleHivemindWebsiteCase function which handles both activation and deactivation scenarios.
const handleHivemindMediaWikiCase = async (platform: any) => {
const platformDoc = await platformService.getPlatformById(platform.platform);
if (!platformDoc) return;
const isActivated = platform.metadata?.activated;
const existingWorkflowId = platformDoc.get('metadata.workflowId');
if (isActivated === true) {
if (!existingWorkflowId) {
const workflowId = await temporalMediaWiki.executeWorkflow(platform.platform);
platformDoc.set('metadata.workflowId', workflowId);
await platformDoc.save();
}
+ } else if (isActivated === false) {
+ if (existingWorkflowId) {
+ await temporalMediaWiki.terminateWorkflow(existingWorkflowId);
+ platformDoc.set('metadata.workflowId', null);
+ await platformDoc.save();
+ }
}
};📝 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.
| const handleHivemindMediaWikiCase = async (platform: any) => { | |
| const platformDoc = await platformService.getPlatformById(platform.platform); | |
| if (!platformDoc) return; | |
| const isActivated = platform.metadata?.activated; | |
| const existingWorkflowId = platformDoc.get('metadata.workflowId'); | |
| if (isActivated === true) { | |
| if (!existingWorkflowId) { | |
| const workflowId = await temporalMediaWiki.executeWorkflow(platform.platform); | |
| platformDoc.set('metadata.workflowId', workflowId); | |
| await platformDoc.save(); | |
| } | |
| } | |
| }; | |
| const handleHivemindMediaWikiCase = async (platform: any) => { | |
| const platformDoc = await platformService.getPlatformById(platform.platform); | |
| if (!platformDoc) return; | |
| const isActivated = platform.metadata?.activated; | |
| const existingWorkflowId = platformDoc.get('metadata.workflowId'); | |
| if (isActivated === true) { | |
| if (!existingWorkflowId) { | |
| const workflowId = await temporalMediaWiki.executeWorkflow(platform.platform); | |
| platformDoc.set('metadata.workflowId', workflowId); | |
| await platformDoc.save(); | |
| } | |
| } else if (isActivated === false) { | |
| if (existingWorkflowId) { | |
| await temporalMediaWiki.terminateWorkflow(existingWorkflowId); | |
| platformDoc.set('metadata.workflowId', null); | |
| await platformDoc.save(); | |
| } | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/services/module.service.ts between lines 142 and 157, the function
handleHivemindMediaWikiCase currently only processes the activation scenario
when isActivated is true. To fix this, add logic to handle the deactivation case
when isActivated is false, similar to handleHivemindWebsiteCase. This should
include clearing or removing the workflowId from platformDoc metadata and saving
the document to maintain consistency in activation state handling.
|
|
||
| if (isActivated === true) { | ||
| if (!existingWorkflowId) { | ||
| const workflowId = await temporalMediaWiki.executeWorkflow(platform.platform); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify workflow ID storage compatibility.
The executeWorkflow method likely returns a WorkflowHandle object, but this line stores it directly as workflowId. Ensure that the stored value is the correct string identifier that can be used later for workflow termination.
🏁 Script executed:
#!/bin/bash
# Description: Check how workflow IDs are typically stored and retrieved in the codebase
# Expected: Confirm the correct format for storing workflow identifiers
# Search for other workflow ID storage patterns
rg -A 5 -B 5 "workflowId.*=" --type ts
# Search for workflow ID retrieval patterns
rg -A 5 -B 5 "metadata\.workflowId" --type tsLength of output: 1778
🏁 Script executed:
#!/bin/bash
# Locate executeWorkflow definition to inspect its return type
rg -n "executeWorkflow" --type tsLength of output: 260
🏁 Script executed:
#!/bin/bash
# Print the implementation of executeWorkflow in the mediaWiki service
sed -n '1,200p' src/services/temporal/mediaWiki.service.tsLength of output: 1655
Ensure you store the string ID from the WorkflowHandle
The executeWorkflow call returns a WorkflowHandle object, not a bare string. You need to pull out its workflowId property before persisting it. Otherwise you’ll save the entire handle object and pass it to terminateWorkflow, which expects a string.
• File: src/services/module.service.ts
• Around line 152
Suggested diff:
- const workflowId = await temporalMediaWiki.executeWorkflow(platform.platform);
- platformDoc.set('metadata.workflowId', workflowId);
+ const workflowHandle = await temporalMediaWiki.executeWorkflow(platform.platform);
+ const workflowId = workflowHandle.workflowId;
+ platformDoc.set('metadata.workflowId', workflowId);📝 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.
| const workflowId = await temporalMediaWiki.executeWorkflow(platform.platform); | |
| const workflowHandle = await temporalMediaWiki.executeWorkflow(platform.platform); | |
| const workflowId = workflowHandle.workflowId; | |
| platformDoc.set('metadata.workflowId', workflowId); |
🤖 Prompt for AI Agents
In src/services/module.service.ts at line 152, the variable workflowId is
assigned the entire WorkflowHandle object returned by executeWorkflow, but you
need to extract the string ID from this handle. Fix this by accessing the
workflowId property of the returned WorkflowHandle object and assign that string
to the workflowId variable before storing or passing it to terminateWorkflow.
Summary by CodeRabbit
New Features
Bug Fixes