feat: support for editing multiple capture list limits #579
Conversation
RohitR311
added
Type: Bug
WalkthroughThis change extends the ability to update robot workflow "limits" from a single value to multiple values, enabling batch updates at arbitrary workflow positions. The API, backend route, and frontend UI are all updated to support this. The backend now accepts an array of limit updates, applies them to the workflow, and persists them. The frontend dynamically detects all relevant limits in a workflow, displays corresponding input fields, and sends all changes together. Validation is updated to require at least one of several fields, and the process for updating the workflow's target URL is refactored for clarity and efficiency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RobotEditModal
participant API
participant Backend
User->>RobotEditModal: Open robot edit modal
RobotEditModal->>RobotEditModal: Detect all scrapeList limits in workflow
User->>RobotEditModal: Edit multiple limit fields
User->>RobotEditModal: Click Save
RobotEditModal->>API: updateRecording(id, { name, limits[], ... })
API->>Backend: PUT /recordings/:id (with limits[])
Backend->>Backend: Validate and apply all limits to workflow
Backend->>Backend: Update workflow and other fields
Backend-->>API: Success response
API-->>RobotEditModal: Success
RobotEditModal-->>User: Update UI/close modal
Possibly related PRs
Suggested labels
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: 3
🔭 Outside diff range comments (1)
server/src/routes/storage.ts (1)
346-347:⚠️ Potential issueResponse may return stale data
robotwas fetched before theRobot.update(...)call (line 338). Consequently the response might not include the freshly updated workflow or metadata. Return theupdatedRobotinstance instead.-return res.status(200).json({ message: 'Robot updated successfully', robot }); +return res.status(200).json({ message: 'Robot updated successfully', robot: updatedRobot });
🧹 Nitpick comments (4)
src/api/storage.ts (1)
31-36: Prefer an exported, reusable type for thelimitspayload
limitsis now appearing in several layers (frontend, API-client, backend). Duplicating the inline{pairIndex, actionIndex, argIndex, limit}literal in every file invites drift. Consider extracting a dedicated TypeScript interface (e.g.ScrapeListLimitPayload) in a shared location (types.ts) and importing it here and in the React layer so that a change in one place is picked up everywhere by the compiler.server/src/routes/storage.ts (1)
258-262: Validation message uses the wrong field nameThe error message mentions
"target_url", but the field expected in the request body istargetUrl. This may confuse API consumers.-return res.status(400).json({ error: 'Either "name", "limits", "credentials" or "target_url" must be provided.' }); +return res.status(400).json({ error: 'Either "name", "limits", "credentials" or "targetUrl" must be provided.' });src/components/robot/RobotEdit.tsx (2)
136-160:findScrapeListLimitsoverlooks non-zero arg indicesThe current scan only inspects
action.args[0]. If a future workflow places thelimitinside another argument (e.g. index 1 for an options object) it will be ignored. Iterating over all args increases robustness.- if (action.action === 'scrapeList' && action.args && action.args.length > 0) { - const arg = action.args[0]; + if (action.action === 'scrapeList' && Array.isArray(action.args)) { + action.args.forEach((arg, argIndex) => { + if (arg && typeof arg === 'object' && 'limit' in arg) { + limits.push({ pairIndex, actionIndex, argIndex, currentLimit: arg.limit }); + } + }); ...
323-351: State updates risk stale closures
setScrapeListLimitsis invoked inside the functional update ofsetRobot. Because React batches state updates,previnsidesetScrapeListLimitscould reference an outdated array when many quick edits occur. Safer pattern:setRobot(prev => { ... }); setScrapeListLimits(prevLimits => prevLimits.map(...));Trigger the two updates separately to avoid nested-state pitfalls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/src/routes/storage.ts(3 hunks)src/api/storage.ts(1 hunks)src/components/robot/RobotEdit.tsx(7 hunks)
| if (targetUrl) { | ||
| const updatedWorkflow = robot.recording.workflow.map((step) => { | ||
| if (step.where?.url && step.where.url !== "about:blank") { | ||
| step.where.url = targetUrl; | ||
| } | ||
|
|
||
| step.what.forEach((action) => { | ||
| const updatedWorkflow = [...robot.recording.workflow]; | ||
|
|
||
| for (let i = updatedWorkflow.length - 1; i >= 0; i--) { | ||
| const step = updatedWorkflow[i]; | ||
| for (let j = 0; j < step.what.length; j++) { | ||
| const action = step.what[j]; | ||
| if (action.action === "goto" && action.args?.length) { | ||
| action.args[0] = targetUrl; | ||
| } | ||
| }); | ||
|
|
||
| return step; | ||
| }); | ||
|
|
||
| robot.set('recording', { ...robot.recording, workflow: updatedWorkflow }); | ||
| robot.changed('recording', true); | ||
| action.args[0] = targetUrl; | ||
| if (step.where?.url && step.where.url !== "about:blank") { | ||
| step.where.url = targetUrl; | ||
| } | ||
|
|
||
| robot.set('recording', { ...robot.recording, workflow: updatedWorkflow }); | ||
| robot.changed('recording', true); | ||
| i = -1; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| await robot.save(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Multiple persistence calls & brittle loop-breaking logic
-
The workflow is mutated,
robot.set(...)is called andawait robot.save()(line 299), then later a secondRobot.update(...)call is executed (line 338). Two round-trips to the DB are unnecessary and risk race conditions if another request updates the same record in between. -
Breaking out of the outer loop via
i = -1is error-prone and non-idiomatic. A labeledbreakor early‐return would be clearer.
A cleaner approach is to:
• Mutate updatedWorkflow in memory,
• Build the single updates object (including the updated workflow and optionally recording_meta.name),
• Call Robot.update(...) once.
- robot.set('recording', { ...robot.recording, workflow: updatedWorkflow });
- robot.changed('recording', true);
- i = -1;
- break;
+ foundGoto = true;
+ break outer;…and remove the earlier robot.save() entirely.
Committable suggestion skipped: line range outside the PR's diff.
| if (limits && Array.isArray(limits) && limits.length > 0) { | ||
| for (const limitInfo of limits) { | ||
| const { pairIndex, actionIndex, argIndex, limit } = limitInfo; | ||
|
|
||
| const pair = workflow[pairIndex]; | ||
| if (!pair || !pair.what) continue; | ||
|
|
||
| const action = pair.what[actionIndex]; | ||
| if (!action || !action.args) continue; | ||
|
|
||
| const arg = action.args[argIndex]; | ||
| if (!arg || typeof arg !== 'object') continue; | ||
|
|
||
| (arg as { limit: number }).limit = limit; | ||
| } |
There was a problem hiding this comment.
Missing sanitisation for user-supplied limit values
limit is written straight into the workflow without any bounds-checking. A malicious caller could set an arbitrarily large (or negative) number and later crash the interpreter.
- (arg as { limit: number }).limit = limit;
+ const capped = Math.max(1, Math.min(5, Number(limit))); // 5 is the upper-bound used elsewhere
+ (arg as { limit: number }).limit = capped;Add explicit Number.isFinite checks and return 400 on invalid input.
| const renderScrapeListLimitFields = () => { | ||
| if (scrapeListLimits.length === 0) return null; | ||
|
|
||
| return ( | ||
| <> | ||
| <Typography variant="body1" style={{ marginBottom: '20px' }}> | ||
| {t('List Limits')} | ||
| </Typography> | ||
|
|
||
| {scrapeListLimits.map((limitInfo, index) => ( | ||
| <TextField | ||
| key={`limit-${limitInfo.pairIndex}-${limitInfo.actionIndex}`} | ||
| label={`${t('List Limit')} ${index + 1}`} | ||
| type="number" | ||
| value={limitInfo.currentLimit || ''} | ||
| onChange={(e) => { | ||
| const value = parseInt(e.target.value, 10); | ||
| if (value >= 1) { | ||
| handleLimitChange( | ||
| limitInfo.pairIndex, | ||
| limitInfo.actionIndex, | ||
| limitInfo.argIndex, | ||
| value | ||
| ); | ||
| } | ||
| }} | ||
| inputProps={{ min: 1 }} | ||
| style={{ marginBottom: '20px' }} | ||
| /> | ||
| ))} | ||
| </> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
parseInt / empty input edge case & non-unique React keys
-
Clearing a limit field yields
parseInt('', 10) → NaN, which fails thevalue >= 1guard and leaves the UI stuck on the old value. Provide explicit empty-string handling. -
The key omits
argIndex; two limits on different args of the same action would collide.
-const value = parseInt(e.target.value, 10);
-if (value >= 1) {
+const raw = e.target.value;
+const parsed = Number(raw);
+if (raw === '') {
+ handleLimitChange(..., 1); // or omit the update and show validation
+} else if (!Number.isNaN(parsed) && parsed >= 1) {
+ handleLimitChange(..., parsed);
}
...
-key={`limit-${limitInfo.pairIndex}-${limitInfo.actionIndex}`}
+key={`limit-${limitInfo.pairIndex}-${limitInfo.actionIndex}-${limitInfo.argIndex}`}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor