chore: migrate to /api/v1/workflow/:id/execute
#250
Conversation
WalkthroughThis change updates the workflow execution API endpoint from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIv1Router
participant WorkflowService
Client->>APIv1Router: POST /api/v1/workflow/:id/execute
APIv1Router->>WorkflowService: validate & execute workflow(id)
WorkflowService-->>APIv1Router: execution result
APIv1Router-->>Client: response (success or error)
Possibly related PRs
Poem
✨ 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: 0
🧹 Nitpick comments (4)
src/httpServer/routers/apiV1.js (1)
184-185: Consider keeping a backward-compatibility alias for/run.Renaming the endpoint is fine, but any existing consumers that still hit
/workflow/:id/runwill now receive a 404. If you can’t guarantee that all callers have already migrated, adding a thin alias:+// TEMP: keep backward-compat for one release +router.post('/workflow/:id/run', (req, res, next) => + router.handle({ ...req, url: req.url.replace('/run', '/execute') }, res, next))lets you deprecate the old path gracefully instead of breaking clients outright.
Run the quick grep below to confirm there are no remaining references to
/run:#!/bin/bash rg -n "/workflow/.*/run" --glob "*.js" --glob "*.yml" --glob "*.test.js"__tests__/httpServer/apiV1.test.js (2)
161-189: Variable shadowing makes the tests harder to follow.Inside this
describe,let mockWorkflowFnshadows the top-levelconst mockWorkflowFnthat was already bound in thejest.mockfactory.
The double definition is harmless but confusing—a reader must keep track of which mock is in play.You can either reuse the outer mock:
- let workflowSpy - let mockWorkflowFn + let workflowSpyand, in
beforeEach, reset it withmockWorkflowFn.mockReset(),
or rename the inner one (e.g.localWorkflowMock) to avoid the shadow.
194-201: Minor nit – assertion ignores the acceptance semantics of 202.The handler returns a synchronous “completed” result yet advertises
202 Accepted, which semantically implies delayed processing.
Consider using200 OKfor immediate completion or truly deferring execution and returning a job handle with202.
This wasn’t changed in this PR, so treat it as future tech-debt.src/httpServer/swagger/api-v1.yml (1)
64-67: OpenAPI description still says “Executes …” but response code 202 indicates async.Same remark as in the tests: if the server responds only when the workflow is finished, 200/201 is more appropriate; if it is actually async, the example response object should not include
status: completed.
Aligning spec and implementation avoids client confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
__tests__/httpServer/apiV1.test.js(6 hunks)src/httpServer/routers/apiV1.js(1 hunks)src/httpServer/swagger/api-v1.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Playwright Tests
Related #216
Summary by CodeRabbit
/api/v1/workflow/{id}/runto/api/v1/workflow/{id}/execute. All related documentation and tests have been updated to reflect this change.