Skip to content

feat: support for editing multiple capture list limits #579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 6, 2025
Merged

feat: support for editing multiple capture list limits #579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
53d992e
feat: multiple capture list limit
RohitR311 May 1, 2025
1535c05
feat: edit update recording
RohitR311 May 1, 2025
24f3d50
feat: edit multiple list limits, change landing url
RohitR311 May 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 35 additions & 49 deletions server/src/routes/storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,11 +254,11 @@ function handleWorkflowActions(workflow: any[], credentials: Credentials) {
router.put('/recordings/:id', requireSignIn, async (req: AuthenticatedRequest, res) => {
try {
const { id } = req.params;
const { name, limit, credentials, targetUrl } = req.body;
const { name, limits, credentials, targetUrl } = req.body;

// Validate input
if (!name && limit === undefined && !targetUrl) {
return res.status(400).json({ error: 'Either "name", "limit" or "target_url" must be provided.' });
if (!name && !limits && !credentials && !targetUrl) {
return res.status(400).json({ error: 'Either "name", "limits", "credentials" or "target_url" must be provided.' });
}

// Fetch the robot by ID
Expand All @@ -274,22 +274,26 @@ router.put('/recordings/:id', requireSignIn, async (req: AuthenticatedRequest, r
}

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();
Comment on lines 276 to 299
Copy link

@coderabbitai coderabbitai bot May 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Multiple persistence calls & brittle loop-breaking logic

  1. The workflow is mutated, robot.set(...) is called and await robot.save() (line 299), then later a second Robot.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.

  2. Breaking out of the outer loop via i = -1 is error-prone and non-idiomatic. A labeled break or 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.

Expand All @@ -300,38 +304,20 @@ router.put('/recordings/:id', requireSignIn, async (req: AuthenticatedRequest, r
workflow = handleWorkflowActions(workflow, credentials);
}

// Update the limit
if (limit !== undefined) {
// Ensure the workflow structure is valid before updating
if (
workflow.length > 0 &&
workflow[0]?.what?.[0]
) {
// Create a new workflow object with the updated limit
workflow = workflow.map((step, index) => {
if (index === 0) { // Assuming you want to update the first step
return {
...step,
what: step.what.map((action, actionIndex) => {
if (actionIndex === 0) { // Assuming the first action needs updating
return {
...action,
args: (action.args ?? []).map((arg, argIndex) => {
if (argIndex === 0) { // Assuming the first argument needs updating
return { ...arg, limit };
}
return arg;
}),
};
}
return action;
}),
};
}
return step;
});
} else {
return res.status(400).json({ error: 'Invalid workflow structure for updating limit.' });
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;
}
Comment on lines +307 to 321
Copy link

@coderabbitai coderabbitai bot May 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

}

Expand Down
7 changes: 6 additions & 1 deletion src/api/storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,12 @@ export const getStoredRecordings = async (): Promise<string[] | null> => {
}
};

export const updateRecording = async (id: string, data: { name?: string; limit?: number, credentials?: Credentials, targetUrl?: string }): Promise<boolean> => {
export const updateRecording = async (id: string, data: {
name?: string;
limits?: Array<{pairIndex: number, actionIndex: number, argIndex: number, limit: number}>;
credentials?: Credentials;
targetUrl?: string
}): Promise<boolean> => {
try {
const response = await axios.put(`${apiUrl}/storage/recordings/${id}`, data);
if (response.status === 200) {
Expand Down
121 changes: 94 additions & 27 deletions src/components/robot/RobotEdit.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,13 @@ interface GroupedCredentials {
others: string[];
}

interface ScrapeListLimit {
pairIndex: number;
actionIndex: number;
argIndex: number;
currentLimit: number;
}

export const RobotEditModal = ({ isOpen, handleStart, handleClose, initialSettings }: RobotSettingsProps) => {
const { t } = useTranslation();
const [credentials, setCredentials] = useState<Credentials>({});
Expand All @@ -85,6 +92,7 @@ export const RobotEditModal = ({ isOpen, handleStart, handleClose, initialSettin
others: []
});
const [showPasswords, setShowPasswords] = useState<CredentialVisibility>({});
const [scrapeListLimits, setScrapeListLimits] = useState<ScrapeListLimit[]>([]);

const isEmailPattern = (value: string): boolean => {
return value.includes('@');
Expand Down Expand Up @@ -120,9 +128,36 @@ export const RobotEditModal = ({ isOpen, handleStart, handleClose, initialSettin
const extractedCredentials = extractInitialCredentials(robot.recording.workflow);
setCredentials(extractedCredentials);
setCredentialGroups(groupCredentialsByType(extractedCredentials));

findScrapeListLimits(robot.recording.workflow);
}
}, [robot]);

const findScrapeListLimits = (workflow: WhereWhatPair[]) => {
const limits: ScrapeListLimit[] = [];

workflow.forEach((pair, pairIndex) => {
if (!pair.what) return;

pair.what.forEach((action, actionIndex) => {
if (action.action === 'scrapeList' && action.args && action.args.length > 0) {
// Check if first argument has a limit property
const arg = action.args[0];
if (arg && typeof arg === 'object' && 'limit' in arg) {
limits.push({
pairIndex,
actionIndex,
argIndex: 0,
currentLimit: arg.limit
});
}
}
});
});

setScrapeListLimits(limits);
};

function extractInitialCredentials(workflow: any[]): Credentials {
const credentials: Credentials = {};

Expand Down Expand Up @@ -285,20 +320,30 @@ export const RobotEditModal = ({ isOpen, handleStart, handleClose, initialSettin
}));
};

const handleLimitChange = (newLimit: number) => {
const handleLimitChange = (pairIndex: number, actionIndex: number, argIndex: number, newLimit: number) => {
setRobot((prev) => {
if (!prev) return prev;

const updatedWorkflow = [...prev.recording.workflow];
if (
updatedWorkflow.length > 0 &&
updatedWorkflow[0]?.what &&
updatedWorkflow[0].what.length > 0 &&
updatedWorkflow[0].what[0].args &&
updatedWorkflow[0].what[0].args.length > 0 &&
updatedWorkflow[0].what[0].args[0]
updatedWorkflow.length > pairIndex &&
updatedWorkflow[pairIndex]?.what &&
updatedWorkflow[pairIndex].what.length > actionIndex &&
updatedWorkflow[pairIndex].what[actionIndex].args &&
updatedWorkflow[pairIndex].what[actionIndex].args.length > argIndex
) {
updatedWorkflow[0].what[0].args[0].limit = newLimit;
updatedWorkflow[pairIndex].what[actionIndex].args[argIndex].limit = newLimit;

setScrapeListLimits(prev => {
return prev.map(item => {
if (item.pairIndex === pairIndex &&
item.actionIndex === actionIndex &&
item.argIndex === argIndex) {
return { ...item, currentLimit: newLimit };
}
return item;
});
});
}

return { ...prev, recording: { ...prev.recording, workflow: updatedWorkflow } };
Expand Down Expand Up @@ -358,9 +403,6 @@ export const RobotEditModal = ({ isOpen, handleStart, handleClose, initialSettin

return (
<>
{/* <Typography variant="h6" style={{ marginBottom: '20px' }}>
{headerText}
</Typography> */}
{selectors.map((selector, index) => {
const isVisible = showPasswords[selector];

Expand Down Expand Up @@ -393,6 +435,40 @@ export const RobotEditModal = ({ isOpen, handleStart, handleClose, initialSettin
);
};

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' }}
/>
))}
</>
);
};
Comment on lines +438 to +470
Copy link

@coderabbitai coderabbitai bot May 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

parseInt / empty input edge case & non-unique React keys

  1. Clearing a limit field yields parseInt('', 10) → NaN, which fails the value >= 1 guard and leaves the UI stuck on the old value. Provide explicit empty-string handling.

  2. 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.


const handleSave = async () => {
if (!robot) return;

Expand All @@ -412,7 +488,12 @@ export const RobotEditModal = ({ isOpen, handleStart, handleClose, initialSettin

const payload = {
name: robot.recording_meta.name,
limit: robot.recording.workflow[0]?.what[0]?.args?.[0]?.limit,
limits: scrapeListLimits.map(limit => ({
pairIndex: limit.pairIndex,
actionIndex: limit.actionIndex,
argIndex: limit.argIndex,
limit: limit.currentLimit
})),
credentials: credentialsForPayload,
targetUrl: targetUrl,
};
Expand Down Expand Up @@ -468,21 +549,7 @@ export const RobotEditModal = ({ isOpen, handleStart, handleClose, initialSettin
style={{ marginBottom: '20px' }}
/>

{robot.recording.workflow?.[0]?.what?.[0]?.args?.[0]?.limit !== undefined && (
<TextField
label={t('robot_edit.robot_limit')}
type="number"
value={robot.recording.workflow[0].what[0].args[0].limit || ''}
onChange={(e) => {
const value = parseInt(e.target.value, 10);
if (value >= 1) {
handleLimitChange(value);
}
}}
inputProps={{ min: 1 }}
style={{ marginBottom: '20px' }}
/>
)}
{renderScrapeListLimitFields()}

{(Object.keys(credentials).length > 0) && (
<>
Expand Down