fix: abort operation #629
fix: abort operation #629
Conversation
WalkthroughThis update removes the server Dockerfile and refactors several frontend and backend files to improve code formatting, error handling, and function signatures. The abort run logic is enhanced to provide more detailed responses and status updates. Several React component props and handler signatures are updated to require additional arguments for aborting runs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (React)
participant Backend (API)
participant DB
User->>UI (React): Click "Abort Run" (with runId, robotName, browserId)
UI (React)->>Backend (API): POST /runs/abort/:id (with identifiers)
Backend (API)->>DB: Update run status to 'aborting'
alt Run is queued
Backend (API)->>DB: Set status to 'aborted', add finished timestamp and log
Backend (API)->>UI (React): Respond with abort job ID and success
else Run is running
Backend (API)->>Abort Queue: Add abort job
Backend (API)->>UI (React): Respond with abort job ID and success
end
UI (React)->>User: Show notification (success or error)
Possibly related PRs
Suggested reviewers
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
🔭 Outside diff range comments (1)
src/components/run/RunContent.tsx (1)
36-36:⚠️ Potential issueCritical: Function signature inconsistency.
The
abortRunHandlerfunction signature here shows() => void, but according to the changes in other files (Runs.tsx, ColapsibleRow.tsx, RunsTable.tsx), it should be(runId: string, robotName: string, browserId: string) => void. This inconsistency will cause TypeScript compilation errors.Apply this fix:
- abortRunHandler: () => void, + abortRunHandler: (runId: string, robotName: string, browserId: string) => void,You'll also need to update the usage in the component to pass the required parameters.
🧹 Nitpick comments (1)
server/src/routes/storage.ts (1)
153-157: Consider using optional chaining for safer property access.The static analysis tool suggests using optional chaining to improve safety when accessing nested properties.
Apply this diff to use optional chaining:
- if (run.serializableOutput && run.serializableOutput['item-0']) { - formattedRun.data = run.serializableOutput['item-0']; - } else if (run.binaryOutput && run.binaryOutput['item-0']) { - formattedRun.screenshot = run.binaryOutput['item-0']; + if (run.serializableOutput?.['item-0']) { + formattedRun.data = run.serializableOutput['item-0']; + } else if (run.binaryOutput?.['item-0']) { + formattedRun.screenshot = run.binaryOutput['item-0'];🧰 Tools
🪛 Biome (1.9.4)
[error] 153-153: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 155-155: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
server/Dockerfile(0 hunks)server/src/routes/storage.ts(13 hunks)src/components/run/ColapsibleRow.tsx(2 hunks)src/components/run/RunContent.tsx(2 hunks)src/components/run/Runs.tsx(1 hunks)src/components/run/RunsTable.tsx(2 hunks)src/pages/MainPage.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- server/Dockerfile
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/pages/MainPage.tsx (1)
src/api/storage.ts (1)
notifyAboutAbort(187-199)
🪛 Biome (1.9.4)
server/src/routes/storage.ts
[error] 153-153: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 155-155: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (10)
src/components/run/RunContent.tsx (1)
444-449: Good enhancement: Allow aborting queued runs.Expanding the condition to include
'queued'status is a logical improvement that allows users to abort runs before they start executing.src/components/run/Runs.tsx (1)
7-7: LGTM: Function signature correctly updated.The
abortRunHandlersignature now properly requires the three parameters needed for enhanced abort functionality:runId,robotName, andbrowserId.src/components/run/ColapsibleRow.tsx (2)
36-36: LGTM: Function signature correctly updated.The
abortRunHandlersignature properly matches the enhanced abort functionality requiringrunId,robotName, andbrowserIdparameters.
62-64: LGTM: Correct implementation of enhanced abort handler.The function correctly passes the required three parameters from the row data:
runId,name(as robotName), andbrowserId.src/components/run/RunsTable.tsx (2)
56-56: LGTM: Added browserId property for enhanced abort functionality.The
browserIdfield addition to theDatainterface is necessary to support the enhanced abort handler that requires browser identification.
69-69: LGTM: Function signature correctly updated.The
abortRunHandlersignature properly matches the enhanced abort functionality requiringrunId,robotName, andbrowserIdparameters.src/pages/MainPage.tsx (2)
52-63: LGTM! Improved function signature and self-containment.The updated
abortRunHandlersignature with explicit parameters (runId,robotName,browserId) makes the function more self-contained and less dependent on component state. Using the passedrobotNameparameter instead ofrunningRecordingNamestate improves consistency, and addingsetRerenderRuns(true)ensures proper UI updates after the abort operation.
160-160: Simplified prop passing aligns with function signature change.The direct passing of
abortRunHandlerfunction is consistent with the updated signature that now accepts the required parameters (runId,robotName,browserId) that will be provided by theRunscomponent.server/src/routes/storage.ts (2)
890-949: Excellent enhancement of abort endpoint with comprehensive validation and error handling.The updated abort endpoint significantly improves functionality with:
- Proper authorization: Validates user authentication and run ownership
- Status validation: Only allows aborting runs with 'running' or 'queued' status
- Differentiated handling: Queued runs are immediately marked as 'aborted', while running runs are queued for proper termination
- Comprehensive error responses: Returns appropriate HTTP status codes and detailed error messages
- Improved logging: Better tracking of abort operations
The implementation correctly handles the edge case where queued runs don't need complex termination logic and can be immediately aborted.
171-249: Approve code formatting improvements in handleWorkflowActions function.The reformatting of the
handleWorkflowActionsfunction significantly improves readability while maintaining the same business logic for credential injection and workflow processing.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
server/src/routes/storage.ts (1)
608-670:⚠️ Potential issueSyntax error – double
elsechain makes the file uncompilableA second
elseappears immediately after a completeif/elseblock, which produces a parser error (Expected a statement here; unreachable code).
Nothing after the firstreturn res.send({...})can be reached, so the active-browser reuse branch never runs.- } else { - const browserId = uuid(); - … - return res.send({...}); - } else { - const browserId = getActiveBrowserIdByState(req.user.id, "run") - … - return res.send({...}); - } + } else { + // Try to reuse an active browser first; if none, fall back to a random UUID + const browserId = + getActiveBrowserIdByState(req.user.id, 'run') ?? uuid(); + + await Run.create({ … }); + + return res.send({ + browserId, + runId, + robotMetaId: recording.recording_meta.id, + queued: true, + }); + }Remove the first queued branch or merge the two paths as shown. Without this fix the server will fail to start.
🧰 Tools
🪛 Biome (1.9.4)
[error] 633-633: Expected a statement but instead found 'else'.
Expected a statement here.
(parse)
[error] 634-669: This code is unreachable
(lint/correctness/noUnreachable)
🧹 Nitpick comments (3)
server/src/routes/storage.ts (3)
993-1004: Minor: missing await-safety around queue creation
pgBoss.createQueuereturns a promise; failures inside it are already caught, but any rejection increateQueuebeforesendwould currently be swallowed. Wrap both calls in a singletry/awaitblock (or keep as is but add its own try/catch).
139-157: Use optional chaining for brevityStatic-analysis correctly flags the nested checks; optional chaining reduces noise:
- if (run.serializableOutput && run.serializableOutput['item-0']) { - formattedRun.data = run.serializableOutput['item-0']; - } else if (run.binaryOutput && run.binaryOutput['item-0']) { - formattedRun.screenshot = run.binaryOutput['item-0']; + if (run.serializableOutput?.['item-0']) { + formattedRun.data = run.serializableOutput['item-0']; + } else if (run.binaryOutput?.['item-0']) { + formattedRun.screenshot = run.binaryOutput['item-0']; }🧰 Tools
🪛 Biome (1.9.4)
[error] 153-153: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 155-155: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
25-61: Deep-clone withJSON.stringifydrops non-serialisable fields
processWorkflowActionsusesJSON.parse(JSON.stringify(workflow)). If a workflow ever contains functions, Dates, or circular refs they will be silently lost or crash. ConsiderstructuredClone(Node 18+) orlodash.cloneDeepfor safer cloning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/src/routes/storage.ts(11 hunks)src/components/run/RunContent.tsx(2 hunks)src/pages/MainPage.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/run/RunContent.tsx
- src/pages/MainPage.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
server/src/routes/storage.ts
[error] 153-153: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 155-155: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 633-633: Expected a statement but instead found 'else'.
Expected a statement here.
(parse)
[error] 634-669: This code is unreachable
(lint/correctness/noUnreachable)
| if (!['running', 'queued'].includes(run.status)) { | ||
| return res.status(400).send({ | ||
| error: `Cannot abort run with status: ${run.status}` | ||
| }); | ||
| } | ||
|
|
||
| await run.update({ | ||
| status: 'aborting' | ||
| }); | ||
|
|
||
| if (run.status === 'queued') { | ||
| await run.update({ | ||
| status: 'aborted', | ||
| finishedAt: new Date().toLocaleString(), | ||
| log: 'Run aborted while queued' | ||
| }); | ||
|
|
||
| return res.send({ success: true, message: 'Queued run aborted' }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Logic bug – run.status is inspected after you mutate it
await run.update({ status: 'aborting' }) mutates the instance, so the subsequent
if (run.status === 'queued') { … }can never be true. Capture the original status before the update:
- await run.update({ status: 'aborting' });
- if (run.status === 'queued') {
+ const previousStatus = run.status as string;
+ await run.update({ status: 'aborting' });
+ if (previousStatus === 'queued') {Without this, queued runs are never marked aborted, and no response is sent to the client.
🤖 Prompt for AI Agents
In server/src/routes/storage.ts around lines 970 to 989, the code updates
run.status to 'aborting' before checking if the original status was 'queued',
causing the condition to never be true. To fix this, store the original
run.status in a variable before the update call, then use that variable in the
subsequent if condition to correctly handle queued runs and send the appropriate
response.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores