fix: handle displayed input texts #435
Conversation
WalkthroughThe changes update several modules associated with robot workflow processing. In the storage routes, the update logic now directly maps over workflow actions with credential encryption and consolidates robot updates. The workflow generator introduces a new input state interface and a two-pass optimization process for handling input actions, along with enhanced logging of decrypted keys. The credential extraction in the robot edit component is simplified by removing intermediate state tracking and redundant logic. Changes
Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Nitpick comments (1)
server/src/workflow-management/classes/Generator.ts (1)
1166-1224: Check coverage for special key sequences.The logic for handling typed characters, Backspace, and Delete is well-structured. However, consider how other special keys (e.g., "Enter", "Tab", arrow keys) might arise in the future. If you plan to handle them similarly, you may need additional cases here. Also ensure the final input state matches the user’s actual typed outcome for all possible key events.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/src/routes/storage.ts(2 hunks)server/src/workflow-management/classes/Generator.ts(5 hunks)src/components/robot/RobotEdit.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
server/src/workflow-management/classes/Generator.ts
[error] 1229-1229: This code is unreachable
(lint/correctness/noUnreachable)
🔇 Additional comments (7)
server/src/workflow-management/classes/Generator.ts (4)
25-25: Import usage acknowledgment.The new import for
decryptandencryptaligns with the updated logic for handling encrypted keypress actions. This inclusion appears intentional and consistent with the subsequent usage inoptimizeWorkflow.
45-50: Good addition of theInputStateinterface.This interface provides clear structure for tracking input fields' selector, value, type, and cursor position. Consider introducing an enum for the
typefield if the value set is known (e.g., "text", "password"), but otherwise, this approach is fine.
1135-1165: Validate removal of click actions.The code removes click actions entirely whenever a cursor position is derived, shifting responsibility for text entry to the second pass. Verify that removing these click actions will not skip any subsequent workflows that rely on the actual mouse click. If these clicks are purely for positioning the input cursor, this is fine. Otherwise, consider preserving them and marking them distinctly.
1225-1247: Consolidate final input state as a singletypeaction.The second pass effectively creates a single
typeaction reflecting the final text for each selector. This can significantly reduce duplication in the workflow, but be aware that intermediate states (e.g., partial text) will be lost. If partial keystroke playback is required, you might want to preserve them.(Static analysis note: The tool flagged code at line 1229 as “unreachable.” This loop appears reachable if
state.valueis truthy. Consider verifying that no logic path inadvertently bypasses it, or dismiss the warning as a false positive.)🧰 Tools
🪛 Biome (1.9.4)
[error] 1229-1229: This code is unreachable
(lint/correctness/noUnreachable)
src/components/robot/RobotEdit.tsx (1)
127-159: Straightforward credential extraction.The
extractInitialCredentialsfunction nicely centralizes logic for capturing the final text typed into each selector. Consider clarifying which actions are covered (only'type'in this flow) and adding defensive checks for edge cases (e.g., empty selector strings). Otherwise, this refactor improves maintainability by removing unnecessary intermediate state.server/src/routes/storage.ts (2)
170-190: Efficient credential encryption inupdateTypeActionsInWorkflow.Transforming 'type' actions in-place and replacing plaintext values with encrypted credentials is a clean approach. Ensure that all references to
action.argsremain valid if more arguments are inserted in the future. Also confirm that ignoring any other action types is intentional and will not skip potential credentials in 'press' actions.
259-275: Grouped updates object for name and workflow changes.Combining updates to
recordingandrecording_metainto a singleRobot.updatecall is more concise and avoids partial saves. Validate that you don’t unintentionally overwrite fields inrecording_metaother thanname. If further fields appear, ensure they are merged carefully.
src/components/robot/RobotEdit.tsx
Outdated
| console.log(extractedCredentials); | ||
| setCredentials(extractedCredentials); |
There was a problem hiding this comment.
Remove or gate debug logs before production.
While logging extracted credentials can be helpful in debugging, it may inadvertently leak sensitive data (like passwords). It’s recommended to remove or conditionally guard this log statement to avoid exposing PII or credentials in logs.
- console.log(extractedCredentials);
+ // console.log(extractedCredentials); // Remove before production📝 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.
| console.log(extractedCredentials); | |
| setCredentials(extractedCredentials); | |
| // console.log(extractedCredentials); // Remove before production | |
| setCredentials(extractedCredentials); |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/robot/RobotEdit.tsx (1)
337-340:⚠️ Potential issueRemove or sanitize error logging of sensitive data.
The error logging could potentially expose sensitive credential information. Consider sanitizing the error object before logging.
- console.error('Error updating robot:', error); + console.error('Error updating robot settings. Please check the input values and try again.');
🧹 Nitpick comments (3)
src/components/robot/RobotEdit.tsx (3)
144-149: Initialize credentials with null instead of empty strings.Initializing credentials with empty strings could make it difficult to distinguish between unset and intentionally empty values.
- value: '', - type: '' + value: null, + type: null
331-333: Replace hard page reload with graceful state update.The hard page reload after successful update is disruptive to user experience. Consider updating the state locally instead.
- setTimeout(() => { - window.location.reload(); - }, 1000); + // Update local state if needed + getRobot(); // Fetch fresh data
304-341: Add loading state during save operation.The save operation lacks visual feedback while in progress. Consider adding a loading state to improve user experience.
+ const [isSaving, setIsSaving] = useState(false); const handleSave = async () => { if (!robot) return; + setIsSaving(true); try { // ... existing save logic ... } catch (error) { notify('error', t('robot_edit.notifications.update_error')); console.error('Error updating robot settings. Please check the input values and try again.'); + } finally { + setIsSaving(false); } };Then update the save button:
- <Button variant="contained" color="primary" onClick={handleSave}> + <Button + variant="contained" + color="primary" + onClick={handleSave} + disabled={isSaving} + > - {t('robot_edit.save')} + {isSaving ? t('robot_edit.saving') : t('robot_edit.save')} </Button>
src/components/robot/RobotEdit.tsx
Outdated
| const value: string = action.args[1]; | ||
| const type: string = action.args[2]; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for type information from action arguments.
The code assumes action.args[2] contains valid type information without validation. This could lead to undefined behavior if the type argument is missing or invalid.
- const type: string = action.args[2];
+ const type: string = action.args[2] || 'text'; // Default to 'text' if type is not specified📝 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 value: string = action.args[1]; | |
| const type: string = action.args[2]; | |
| const value: string = action.args[1]; | |
| - const type: string = action.args[2]; | |
| + const type: string = action.args[2] || 'text'; // Default to 'text' if type is not specified |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/src/workflow-management/classes/Generator.ts (1)
437-528: Improve cursor position calculation performance.The cursor position calculation is complex but necessary. However, the current implementation creates and removes DOM elements for each character position, which could be optimized.
Consider caching the mirror element and reusing it across calculations instead of creating/removing it for each position check.
const getCursorPosition = (element: any, clickX: any) => { const text = element.value; + let mirror: HTMLElement | null = null; - const mirror = document.createElement('div'); + if (!mirror) { + mirror = document.createElement('div'); + const style = window.getComputedStyle(element); + mirror.style.cssText = ` + font: ${style.font}; + line-height: ${style.lineHeight}; + padding: ${style.padding}; + border: ${style.border}; + box-sizing: ${style.boxSizing}; + white-space: ${style.whiteSpace}; + overflow-wrap: ${style.overflowWrap}; + position: absolute; + top: -9999px; + left: -9999px; + width: ${element.offsetWidth}px; + `; + document.body.appendChild(mirror); + } - const style = window.getComputedStyle(element); - mirror.style.cssText = ` - font: ${style.font}; - line-height: ${style.lineHeight}; - padding: ${style.padding}; - border: ${style.border}; - box-sizing: ${style.boxSizing}; - white-space: ${style.whiteSpace}; - overflow-wrap: ${style.overflowWrap}; - position: absolute; - top: -9999px; - left: -9999px; - width: ${element.offsetWidth}px; - `; - document.body.appendChild(mirror); // ... rest of the function - document.body.removeChild(mirror); + if (mirror && mirror.parentNode) { + mirror.parentNode.removeChild(mirror); + } return bestIndex; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/workflow-management/classes/Generator.ts(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
server/src/workflow-management/classes/Generator.ts
[error] 1197-1197: This code is unreachable
(lint/correctness/noUnreachable)
🔇 Additional comments (1)
server/src/workflow-management/classes/Generator.ts (1)
45-50: LGTM! Well-structured interface for input state management.The
InputStateinterface provides a clear contract with all necessary properties for tracking input states.
| for (const pair of this.workflowRecord.workflow) { | ||
| for (let i = 0; i < pair.what.length; i++) { | ||
| const condition = pair.what[i]; | ||
|
|
||
| if (condition.action === 'press' && condition.args) { | ||
| const [selector, encryptedKey, type] = condition.args; | ||
| const key = decrypt(encryptedKey); | ||
|
|
||
| console.log(`Selector: ${selector}, Key: ${key}`); | ||
| } | ||
|
|
||
| if (condition.action === 'click' && condition.args) { | ||
| console.log("Click args: ", condition.args); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Security concern: Remove sensitive key logging.
The logging of decrypted keys could expose sensitive information in logs.
Remove or mask the sensitive information in logs:
- console.log(`Selector: ${selector}, Key: ${key}`);
+ console.log(`Selector: ${selector}, Key: ${key.replace(/./g, '*')}`);📝 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.
| for (const pair of this.workflowRecord.workflow) { | |
| for (let i = 0; i < pair.what.length; i++) { | |
| const condition = pair.what[i]; | |
| if (condition.action === 'press' && condition.args) { | |
| const [selector, encryptedKey, type] = condition.args; | |
| const key = decrypt(encryptedKey); | |
| console.log(`Selector: ${selector}, Key: ${key}`); | |
| } | |
| if (condition.action === 'click' && condition.args) { | |
| console.log("Click args: ", condition.args); | |
| } | |
| } | |
| } | |
| for (const pair of this.workflowRecord.workflow) { | |
| for (let i = 0; i < pair.what.length; i++) { | |
| const condition = pair.what[i]; | |
| if (condition.action === 'press' && condition.args) { | |
| const [selector, encryptedKey, type] = condition.args; | |
| const key = decrypt(encryptedKey); | |
| console.log(`Selector: ${selector}, Key: ${key.replace(/./g, '*')}`); | |
| } | |
| if (condition.action === 'click' && condition.args) { | |
| console.log("Click args: ", condition.args); | |
| } | |
| } | |
| } |
| const inputStates = new Map<string, InputState>(); | ||
|
|
||
| for (const pair of workflow.workflow) { | ||
| pair.what.forEach((condition, index) => { | ||
| if (condition.action === 'press') { | ||
| if (condition.args && condition.args[1]) { | ||
| if (!input.selector) { | ||
| input.selector = condition.args[0]; | ||
| } | ||
| if (input.selector === condition.args[0]) { | ||
| input.actionCounter++; | ||
| if (condition.args[1].length === 1) { | ||
| input.value = input.value + condition.args[1]; | ||
| } else if (condition.args[1] === 'Backspace') { | ||
| input.value = input.value.slice(0, -1); | ||
| } else if (condition.args[1] !== 'Shift') { | ||
| pushTheOptimizedAction(pair, index); | ||
| pair.what.splice(index + 1, 0, { | ||
| action: 'waitForLoadState', | ||
| args: ['networkidle'], | ||
| }) | ||
| input = { selector: '', value: '', type: '', actionCounter: 0 }; | ||
| } | ||
| let currentIndex = 0; | ||
|
|
||
| while (currentIndex < pair.what.length) { | ||
| const condition = pair.what[currentIndex]; | ||
|
|
||
| if (condition.action === 'click' && condition.args?.[2]?.cursorIndex !== undefined) { | ||
| const selector = condition.args[0]; | ||
| const cursorIndex = condition.args[2].cursorIndex; | ||
|
|
||
| let state = inputStates.get(selector) || { | ||
| selector, | ||
| value: '', | ||
| type: 'text', | ||
| cursorPosition: -1 | ||
| }; | ||
|
|
||
| state.cursorPosition = cursorIndex; | ||
| inputStates.set(selector, state); | ||
|
|
||
| pair.what.splice(currentIndex, 1); | ||
| continue; | ||
| } | ||
|
|
||
| if (condition.action === 'press' && condition.args?.[1]) { | ||
| const [selector, encryptedKey, type] = condition.args; | ||
| const key = decrypt(encryptedKey); | ||
|
|
||
| let state = inputStates.get(selector); | ||
| if (!state) { | ||
| state = { | ||
| selector, | ||
| value: '', | ||
| type: type || 'text', | ||
| cursorPosition: -1 | ||
| }; | ||
| } else { | ||
| state.type = type || state.type; | ||
| } | ||
|
|
||
| if (key.length === 1) { | ||
| if (state.cursorPosition === -1) { | ||
| state.value += key; | ||
| } else { | ||
| pushTheOptimizedAction(pair, index); | ||
| input = { | ||
| selector: condition.args[0], | ||
| value: condition.args[1], | ||
| type: condition.args[2], | ||
| actionCounter: 1, | ||
| }; | ||
| state.value = | ||
| state.value.slice(0, state.cursorPosition) + | ||
| key + | ||
| state.value.slice(state.cursorPosition); | ||
| state.cursorPosition++; | ||
| } | ||
| } else if (key === 'Backspace') { | ||
| if (state.cursorPosition > 0) { | ||
| state.value = | ||
| state.value.slice(0, state.cursorPosition - 1) + | ||
| state.value.slice(state.cursorPosition); | ||
| state.cursorPosition--; | ||
| } else if (state.cursorPosition === -1 && state.value.length > 0) { | ||
| state.value = state.value.slice(0, -1); | ||
| } | ||
| } else if (key === 'Delete') { | ||
| if (state.cursorPosition >= 0 && state.cursorPosition < state.value.length) { | ||
| state.value = | ||
| state.value.slice(0, state.cursorPosition) + | ||
| state.value.slice(state.cursorPosition + 1); | ||
| } else if (state.cursorPosition === -1 && state.value.length > 0) { | ||
| state.value = state.value.slice(0, -1); | ||
| } | ||
| } | ||
| } else { | ||
| if (input.value.length !== 0) { | ||
| pushTheOptimizedAction(pair, index); | ||
| // clear the input | ||
| input = { selector: '', value: '', type: '', actionCounter: 0 }; | ||
| } | ||
|
|
||
| inputStates.set(selector, state); | ||
|
|
||
| pair.what.splice(currentIndex, 1); | ||
| continue; | ||
| } | ||
| }); | ||
|
|
||
| currentIndex++; | ||
| } | ||
| } | ||
|
|
||
| for (const [selector, state] of inputStates.entries()) { | ||
| if (state.value) { | ||
| for (let i = workflow.workflow.length - 1; i >= 0; i--) { | ||
| const pair = workflow.workflow[i]; | ||
|
|
||
| pair.what.push({ | ||
| action: 'type', | ||
| args: [selector, encrypt(state.value), state.type] | ||
| }, { | ||
| action: 'waitForLoadState', | ||
| args: ['networkidle'] | ||
| }); | ||
|
|
||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return workflow; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix unreachable code and add error handling.
The workflow optimization logic has two issues:
- The code at line 1197 is unreachable as indicated by static analysis.
- Missing error handling for potential edge cases.
Apply these improvements:
for (const [selector, state] of inputStates.entries()) {
if (state.value) {
- for (let i = workflow.workflow.length - 1; i >= 0; i--) {
- const pair = workflow.workflow[i];
-
- pair.what.push({
+ if (workflow.workflow.length > 0) {
+ const lastPair = workflow.workflow[workflow.workflow.length - 1];
+ lastPair.what.push({
action: 'type',
args: [selector, encrypt(state.value), state.type]
}, {
action: 'waitForLoadState',
args: ['networkidle']
});
-
- break;
+ } else {
+ logger.log('warn', `No workflow pairs found for selector: ${selector}`);
}
}
}📝 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 inputStates = new Map<string, InputState>(); | |
| for (const pair of workflow.workflow) { | |
| pair.what.forEach((condition, index) => { | |
| if (condition.action === 'press') { | |
| if (condition.args && condition.args[1]) { | |
| if (!input.selector) { | |
| input.selector = condition.args[0]; | |
| } | |
| if (input.selector === condition.args[0]) { | |
| input.actionCounter++; | |
| if (condition.args[1].length === 1) { | |
| input.value = input.value + condition.args[1]; | |
| } else if (condition.args[1] === 'Backspace') { | |
| input.value = input.value.slice(0, -1); | |
| } else if (condition.args[1] !== 'Shift') { | |
| pushTheOptimizedAction(pair, index); | |
| pair.what.splice(index + 1, 0, { | |
| action: 'waitForLoadState', | |
| args: ['networkidle'], | |
| }) | |
| input = { selector: '', value: '', type: '', actionCounter: 0 }; | |
| } | |
| let currentIndex = 0; | |
| while (currentIndex < pair.what.length) { | |
| const condition = pair.what[currentIndex]; | |
| if (condition.action === 'click' && condition.args?.[2]?.cursorIndex !== undefined) { | |
| const selector = condition.args[0]; | |
| const cursorIndex = condition.args[2].cursorIndex; | |
| let state = inputStates.get(selector) || { | |
| selector, | |
| value: '', | |
| type: 'text', | |
| cursorPosition: -1 | |
| }; | |
| state.cursorPosition = cursorIndex; | |
| inputStates.set(selector, state); | |
| pair.what.splice(currentIndex, 1); | |
| continue; | |
| } | |
| if (condition.action === 'press' && condition.args?.[1]) { | |
| const [selector, encryptedKey, type] = condition.args; | |
| const key = decrypt(encryptedKey); | |
| let state = inputStates.get(selector); | |
| if (!state) { | |
| state = { | |
| selector, | |
| value: '', | |
| type: type || 'text', | |
| cursorPosition: -1 | |
| }; | |
| } else { | |
| state.type = type || state.type; | |
| } | |
| if (key.length === 1) { | |
| if (state.cursorPosition === -1) { | |
| state.value += key; | |
| } else { | |
| pushTheOptimizedAction(pair, index); | |
| input = { | |
| selector: condition.args[0], | |
| value: condition.args[1], | |
| type: condition.args[2], | |
| actionCounter: 1, | |
| }; | |
| state.value = | |
| state.value.slice(0, state.cursorPosition) + | |
| key + | |
| state.value.slice(state.cursorPosition); | |
| state.cursorPosition++; | |
| } | |
| } else if (key === 'Backspace') { | |
| if (state.cursorPosition > 0) { | |
| state.value = | |
| state.value.slice(0, state.cursorPosition - 1) + | |
| state.value.slice(state.cursorPosition); | |
| state.cursorPosition--; | |
| } else if (state.cursorPosition === -1 && state.value.length > 0) { | |
| state.value = state.value.slice(0, -1); | |
| } | |
| } else if (key === 'Delete') { | |
| if (state.cursorPosition >= 0 && state.cursorPosition < state.value.length) { | |
| state.value = | |
| state.value.slice(0, state.cursorPosition) + | |
| state.value.slice(state.cursorPosition + 1); | |
| } else if (state.cursorPosition === -1 && state.value.length > 0) { | |
| state.value = state.value.slice(0, -1); | |
| } | |
| } | |
| } else { | |
| if (input.value.length !== 0) { | |
| pushTheOptimizedAction(pair, index); | |
| // clear the input | |
| input = { selector: '', value: '', type: '', actionCounter: 0 }; | |
| } | |
| inputStates.set(selector, state); | |
| pair.what.splice(currentIndex, 1); | |
| continue; | |
| } | |
| }); | |
| currentIndex++; | |
| } | |
| } | |
| for (const [selector, state] of inputStates.entries()) { | |
| if (state.value) { | |
| for (let i = workflow.workflow.length - 1; i >= 0; i--) { | |
| const pair = workflow.workflow[i]; | |
| pair.what.push({ | |
| action: 'type', | |
| args: [selector, encrypt(state.value), state.type] | |
| }, { | |
| action: 'waitForLoadState', | |
| args: ['networkidle'] | |
| }); | |
| break; | |
| } | |
| } | |
| } | |
| return workflow; | |
| } | |
| }; | |
| const inputStates = new Map<string, InputState>(); | |
| for (const pair of workflow.workflow) { | |
| let currentIndex = 0; | |
| while (currentIndex < pair.what.length) { | |
| const condition = pair.what[currentIndex]; | |
| if (condition.action === 'click' && condition.args?.[2]?.cursorIndex !== undefined) { | |
| const selector = condition.args[0]; | |
| const cursorIndex = condition.args[2].cursorIndex; | |
| let state = inputStates.get(selector) || { | |
| selector, | |
| value: '', | |
| type: 'text', | |
| cursorPosition: -1 | |
| }; | |
| state.cursorPosition = cursorIndex; | |
| inputStates.set(selector, state); | |
| pair.what.splice(currentIndex, 1); | |
| continue; | |
| } | |
| if (condition.action === 'press' && condition.args?.[1]) { | |
| const [selector, encryptedKey, type] = condition.args; | |
| const key = decrypt(encryptedKey); | |
| let state = inputStates.get(selector); | |
| if (!state) { | |
| state = { | |
| selector, | |
| value: '', | |
| type: type || 'text', | |
| cursorPosition: -1 | |
| }; | |
| } else { | |
| state.type = type || state.type; | |
| } | |
| if (key.length === 1) { | |
| if (state.cursorPosition === -1) { | |
| state.value += key; | |
| } else { | |
| state.value = | |
| state.value.slice(0, state.cursorPosition) + | |
| key + | |
| state.value.slice(state.cursorPosition); | |
| state.cursorPosition++; | |
| } | |
| } else if (key === 'Backspace') { | |
| if (state.cursorPosition > 0) { | |
| state.value = | |
| state.value.slice(0, state.cursorPosition - 1) + | |
| state.value.slice(state.cursorPosition); | |
| state.cursorPosition--; | |
| } else if (state.cursorPosition === -1 && state.value.length > 0) { | |
| state.value = state.value.slice(0, -1); | |
| } | |
| } else if (key === 'Delete') { | |
| if (state.cursorPosition >= 0 && state.cursorPosition < state.value.length) { | |
| state.value = | |
| state.value.slice(0, state.cursorPosition) + | |
| state.value.slice(state.cursorPosition + 1); | |
| } else if (state.cursorPosition === -1 && state.value.length > 0) { | |
| state.value = state.value.slice(0, -1); | |
| } | |
| } | |
| inputStates.set(selector, state); | |
| pair.what.splice(currentIndex, 1); | |
| continue; | |
| } | |
| currentIndex++; | |
| } | |
| } | |
| for (const [selector, state] of inputStates.entries()) { | |
| if (state.value) { | |
| if (workflow.workflow.length > 0) { | |
| const lastPair = workflow.workflow[workflow.workflow.length - 1]; | |
| lastPair.what.push({ | |
| action: 'type', | |
| args: [selector, encrypt(state.value), state.type] | |
| }, { | |
| action: 'waitForLoadState', | |
| args: ['networkidle'] | |
| }); | |
| } else { | |
| logger.log('warn', `No workflow pairs found for selector: ${selector}`); | |
| } | |
| } | |
| } | |
| return workflow; | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 1197-1197: This code is unreachable
(lint/correctness/noUnreachable)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/robot/RobotEdit.tsx (1)
151-152: Add error handling for malformed actions.The code assumes the action structure is always valid. Consider adding error handling to gracefully handle malformed actions.
Apply this diff to add error handling:
- credentials[selector].value = value; - credentials[selector].type = type; + try { + credentials[selector].value = sanitizeInput(value); + credentials[selector].type = type; + } catch (error) { + console.error(`Failed to process action for selector ${selector}:`, error); + // Set default values or skip the action + credentials[selector] = { value: '', type: 'text' }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/robot/RobotEdit.tsx(1 hunks)
🔇 Additional comments (1)
src/components/robot/RobotEdit.tsx (1)
304-341: LGTM! Well-structured credential handling.The save functionality properly enforces credential types and includes appropriate error handling. The implementation:
- Enforces password type for sensitive fields
- Includes proper error notifications
- Handles the update process gracefully
src/components/robot/RobotEdit.tsx
Outdated
| const value: string = action.args[1]; | ||
| const type: string = action.args[2] || 'text'; |
There was a problem hiding this comment.
Add input validation for action arguments.
The code directly accesses action.args indices without proper validation. While there's a length check, the type of each argument should be validated.
Apply this diff to add proper validation:
- const value: string = action.args[1];
- const type: string = action.args[2] || 'text';
+ const value = typeof action.args[1] === 'string' ? action.args[1] : '';
+ const type = typeof action.args[2] === 'string' ? action.args[2] : 'text';📝 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 value: string = action.args[1]; | |
| const type: string = action.args[2] || 'text'; | |
| const value = typeof action.args[1] === 'string' ? action.args[1] : ''; | |
| const type = typeof action.args[2] === 'string' ? action.args[2] : 'text'; |
| if (!credentials[selector]) { | ||
| credentials[selector] = { | ||
| value: '', | ||
| type: '' | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input sanitization for credential values.
The code directly assigns user input without sanitization, which could lead to security issues if the input contains malicious content.
Consider adding a sanitization function:
+ const sanitizeInput = (input: string): string => {
+ // Add appropriate sanitization logic
+ return input.trim();
+ };
+
if (!credentials[selector]) {
credentials[selector] = {
- value: '',
+ value: sanitizeInput(''),
type: ''
};
}📝 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.
| if (!credentials[selector]) { | |
| credentials[selector] = { | |
| value: '', | |
| type: '' | |
| }; | |
| } | |
| const sanitizeInput = (input: string): string => { | |
| // Add appropriate sanitization logic | |
| return input.trim(); | |
| }; | |
| if (!credentials[selector]) { | |
| credentials[selector] = { | |
| value: sanitizeInput(''), | |
| type: '' | |
| }; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/workflow-management/classes/Generator.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
server/src/workflow-management/classes/Generator.ts
[error] 1180-1180: This code is unreachable
(lint/correctness/noUnreachable)
🔇 Additional comments (3)
server/src/workflow-management/classes/Generator.ts (3)
45-50: Well-structured interface for input state management.The
InputStateinterface provides a clear and comprehensive structure for managing input states, including value, type, and cursor position tracking.
1180-1192: Fix unreachable code and add error handling.The code after the
breakstatement at line 1191 is unreachable.🧰 Tools
🪛 Biome (1.9.4)
[error] 1180-1180: This code is unreachable
(lint/correctness/noUnreachable)
1123-1126: Security concern: Remove sensitive key logging.The logging of decrypted keys could expose sensitive information in logs.
| const positionAndCursor = await page.evaluate( | ||
| ({ selector, coords }) => { | ||
| const getCursorPosition = (element: any, clickX: any) => { | ||
| const text = element.value; | ||
|
|
||
| const mirror = document.createElement('div'); | ||
|
|
||
| const style = window.getComputedStyle(element); | ||
| mirror.style.cssText = ` | ||
| font: ${style.font}; | ||
| line-height: ${style.lineHeight}; | ||
| padding: ${style.padding}; | ||
| border: ${style.border}; | ||
| box-sizing: ${style.boxSizing}; | ||
| white-space: ${style.whiteSpace}; | ||
| overflow-wrap: ${style.overflowWrap}; | ||
| position: absolute; | ||
| top: -9999px; | ||
| left: -9999px; | ||
| width: ${element.offsetWidth}px; | ||
| `; | ||
|
|
||
| document.body.appendChild(mirror); | ||
|
|
||
| const paddingLeft = parseFloat(style.paddingLeft); | ||
| const borderLeft = parseFloat(style.borderLeftWidth); | ||
|
|
||
| const adjustedClickX = clickX - (paddingLeft + borderLeft); | ||
|
|
||
| let bestIndex = 0; | ||
| let bestDiff = Infinity; | ||
|
|
||
| for (let i = 0; i <= text.length; i++) { | ||
| const textBeforeCursor = text.substring(0, i); | ||
| const span = document.createElement('span'); | ||
| span.textContent = textBeforeCursor; | ||
| mirror.innerHTML = ''; | ||
| mirror.appendChild(span); | ||
|
|
||
| const textWidth = span.getBoundingClientRect().width; | ||
|
|
||
| const diff = Math.abs(adjustedClickX - textWidth); | ||
|
|
||
| if (diff < bestDiff) { | ||
| bestIndex = i; | ||
| bestDiff = diff; | ||
| } | ||
| } | ||
|
|
||
| document.body.removeChild(mirror); | ||
|
|
||
| return bestIndex; | ||
| }; | ||
|
|
||
| if (elementPos) { | ||
| const relativeX = coordinates.x - elementPos.x; | ||
| const relativeY = coordinates.y - elementPos.y; | ||
| const element = document.querySelector(selector) as HTMLInputElement | HTMLTextAreaElement; | ||
| if (!element) return null; | ||
|
|
||
| const rect = element.getBoundingClientRect(); | ||
| const relativeX = coords.x - rect.left; | ||
|
|
||
| return { | ||
| rect: { | ||
| x: rect.left, | ||
| y: rect.top | ||
| }, | ||
| cursorIndex: getCursorPosition(element, relativeX) | ||
| }; | ||
| }, | ||
| { selector, coords: coordinates } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for DOM manipulation.
The cursor position calculation uses DOM manipulation but lacks proper error handling. If an error occurs before the mirror element is removed, it could lead to a memory leak.
Apply this improvement:
const positionAndCursor = await page.evaluate(
({ selector, coords }) => {
const getCursorPosition = (element: any, clickX: any) => {
const text = element.value;
const mirror = document.createElement('div');
+ try {
const style = window.getComputedStyle(element);
mirror.style.cssText = `
font: ${style.font};
line-height: ${style.lineHeight};
padding: ${style.padding};
border: ${style.border};
box-sizing: ${style.boxSizing};
white-space: ${style.whiteSpace};
overflow-wrap: ${style.overflowWrap};
position: absolute;
top: -9999px;
left: -9999px;
width: ${element.offsetWidth}px;
`;
document.body.appendChild(mirror);
const paddingLeft = parseFloat(style.paddingLeft);
const borderLeft = parseFloat(style.borderLeftWidth);
const adjustedClickX = clickX - (paddingLeft + borderLeft);
let bestIndex = 0;
let bestDiff = Infinity;
for (let i = 0; i <= text.length; i++) {
const textBeforeCursor = text.substring(0, i);
const span = document.createElement('span');
span.textContent = textBeforeCursor;
mirror.innerHTML = '';
mirror.appendChild(span);
const textWidth = span.getBoundingClientRect().width;
const diff = Math.abs(adjustedClickX - textWidth);
if (diff < bestDiff) {
bestIndex = i;
bestDiff = diff;
}
}
return bestIndex;
+ } finally {
+ if (mirror.parentNode) {
+ document.body.removeChild(mirror);
+ }
+ }
- document.body.removeChild(mirror);
};📝 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 positionAndCursor = await page.evaluate( | |
| ({ selector, coords }) => { | |
| const getCursorPosition = (element: any, clickX: any) => { | |
| const text = element.value; | |
| const mirror = document.createElement('div'); | |
| const style = window.getComputedStyle(element); | |
| mirror.style.cssText = ` | |
| font: ${style.font}; | |
| line-height: ${style.lineHeight}; | |
| padding: ${style.padding}; | |
| border: ${style.border}; | |
| box-sizing: ${style.boxSizing}; | |
| white-space: ${style.whiteSpace}; | |
| overflow-wrap: ${style.overflowWrap}; | |
| position: absolute; | |
| top: -9999px; | |
| left: -9999px; | |
| width: ${element.offsetWidth}px; | |
| `; | |
| document.body.appendChild(mirror); | |
| const paddingLeft = parseFloat(style.paddingLeft); | |
| const borderLeft = parseFloat(style.borderLeftWidth); | |
| const adjustedClickX = clickX - (paddingLeft + borderLeft); | |
| let bestIndex = 0; | |
| let bestDiff = Infinity; | |
| for (let i = 0; i <= text.length; i++) { | |
| const textBeforeCursor = text.substring(0, i); | |
| const span = document.createElement('span'); | |
| span.textContent = textBeforeCursor; | |
| mirror.innerHTML = ''; | |
| mirror.appendChild(span); | |
| const textWidth = span.getBoundingClientRect().width; | |
| const diff = Math.abs(adjustedClickX - textWidth); | |
| if (diff < bestDiff) { | |
| bestIndex = i; | |
| bestDiff = diff; | |
| } | |
| } | |
| document.body.removeChild(mirror); | |
| return bestIndex; | |
| }; | |
| if (elementPos) { | |
| const relativeX = coordinates.x - elementPos.x; | |
| const relativeY = coordinates.y - elementPos.y; | |
| const element = document.querySelector(selector) as HTMLInputElement | HTMLTextAreaElement; | |
| if (!element) return null; | |
| const rect = element.getBoundingClientRect(); | |
| const relativeX = coords.x - rect.left; | |
| return { | |
| rect: { | |
| x: rect.left, | |
| y: rect.top | |
| }, | |
| cursorIndex: getCursorPosition(element, relativeX) | |
| }; | |
| }, | |
| { selector, coords: coordinates } | |
| ); | |
| const positionAndCursor = await page.evaluate( | |
| ({ selector, coords }) => { | |
| const getCursorPosition = (element: any, clickX: any) => { | |
| const text = element.value; | |
| const mirror = document.createElement('div'); | |
| try { | |
| const style = window.getComputedStyle(element); | |
| mirror.style.cssText = ` | |
| font: ${style.font}; | |
| line-height: ${style.lineHeight}; | |
| padding: ${style.padding}; | |
| border: ${style.border}; | |
| box-sizing: ${style.boxSizing}; | |
| white-space: ${style.whiteSpace}; | |
| overflow-wrap: ${style.overflowWrap}; | |
| position: absolute; | |
| top: -9999px; | |
| left: -9999px; | |
| width: ${element.offsetWidth}px; | |
| `; | |
| document.body.appendChild(mirror); | |
| const paddingLeft = parseFloat(style.paddingLeft); | |
| const borderLeft = parseFloat(style.borderLeftWidth); | |
| const adjustedClickX = clickX - (paddingLeft + borderLeft); | |
| let bestIndex = 0; | |
| let bestDiff = Infinity; | |
| for (let i = 0; i <= text.length; i++) { | |
| const textBeforeCursor = text.substring(0, i); | |
| const span = document.createElement('span'); | |
| span.textContent = textBeforeCursor; | |
| mirror.innerHTML = ''; | |
| mirror.appendChild(span); | |
| const textWidth = span.getBoundingClientRect().width; | |
| const diff = Math.abs(adjustedClickX - textWidth); | |
| if (diff < bestDiff) { | |
| bestIndex = i; | |
| bestDiff = diff; | |
| } | |
| } | |
| return bestIndex; | |
| } finally { | |
| if (mirror.parentNode) { | |
| document.body.removeChild(mirror); | |
| } | |
| } | |
| }; | |
| const element = document.querySelector(selector) as HTMLInputElement | HTMLTextAreaElement; | |
| if (!element) return null; | |
| const rect = element.getBoundingClientRect(); | |
| const relativeX = coords.x - rect.left; | |
| return { | |
| rect: { | |
| x: rect.left, | |
| y: rect.top | |
| }, | |
| cursorIndex: getCursorPosition(element, relativeX) | |
| }; | |
| }, | |
| { selector, coords: coordinates } | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/robot/RobotEdit.tsx (1)
152-165:⚠️ Potential issueAdd input validation and sanitization for full word type actions.
The code directly uses input values without validation or sanitization, which could lead to security issues.
if (action.action === 'type' && action.args?.length >= 2 && typeof action.args[1] === 'string' && action.args[1].length > 1) { + const sanitizedValue = sanitizeInput(action.args[1]); + const validatedType = validateInputType(action.args[2]); if (!credentials[selector]) { credentials[selector] = { - value: action.args[1], - type: action.args[2] || 'text' + value: sanitizedValue, + type: validatedType }; }
🧹 Nitpick comments (2)
src/components/robot/RobotEdit.tsx (2)
126-126: Add proper type definitions for the workflow parameter.The
any[]type is too permissive and could lead to runtime errors. Consider defining a proper interface for the workflow structure.-function extractInitialCredentials(workflow: any[]): Credentials { +interface WorkflowAction { + action?: string; + args?: (string | { limit?: number })[]; +} + +interface WorkflowStep { + what?: WorkflowAction[]; +} + +function extractInitialCredentials(workflow: WorkflowStep[]): Credentials {
196-211: Optimize the nested loop structure.The nested while loop for processing character sequences could be refactored into a separate function for better readability and maintainability.
+ function processCharacterSequence(startIndex: number): { value: string, endIndex: number } { + let processedValue = currentValue; + let currentIndex = startIndex; + + while (currentIndex < step.what.length) { + const nextAction = step.what[currentIndex]; + if (!isValidSequenceAction(nextAction, selector)) break; + + processedValue = updateValueWithCharacter(processedValue, nextAction.args[1]); + currentIndex++; + } + + return { value: processedValue, endIndex: currentIndex }; + } + - let j = i + 1; - while (j < step.what.length) { - const nextAction = step.what[j]; - // ... existing code ... - j++; - } + const { value, endIndex } = processCharacterSequence(i + 1); + currentValue = value; + i = endIndex;
| if (currentSelector && currentValue) { | ||
| credentials[currentSelector] = { | ||
| value: currentValue, | ||
| type: currentType | ||
| type: currentType || 'text' | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation before final credential assignment.
The final credential assignment should validate both the value and type before storing.
if (currentSelector && currentValue) {
+ const trimmedValue = currentValue.trim();
+ if (trimmedValue.length === 0) return;
+
credentials[currentSelector] = {
- value: currentValue,
+ value: trimmedValue,
type: currentType || 'text'
};
}📝 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.
| if (currentSelector && currentValue) { | |
| credentials[currentSelector] = { | |
| value: currentValue, | |
| type: currentType | |
| type: currentType || 'text' | |
| }; | |
| } | |
| if (currentSelector && currentValue) { | |
| const trimmedValue = currentValue.trim(); | |
| if (trimmedValue.length === 0) return; | |
| credentials[currentSelector] = { | |
| value: trimmedValue, | |
| type: currentType || 'text' | |
| }; | |
| } |
| if (!action.action || !action.args?.[0]) { | ||
| i++; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Add proper null checks and bounds validation.
The current checks are insufficient. Add comprehensive validation for the action object structure.
- if (!action.action || !action.args?.[0]) {
+ if (!action?.action || !Array.isArray(action?.args) || action.args.length === 0) {
i++;
continue;
}📝 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.
| if (!action.action || !action.args?.[0]) { | |
| i++; | |
| continue; | |
| } | |
| if (!action?.action || !Array.isArray(action?.args) || action.args.length === 0) { | |
| i++; | |
| continue; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/src/routes/storage.ts (2)
166-254: Consider breaking down the complex logic into smaller functions.The function has high cyclomatic complexity with multiple nested conditions, making it harder to maintain and test. Consider extracting the action processing logic into separate helper functions.
function handleWorkflowActions(workflow: any[], credentials: Credentials) { + function processCredentialAction(action: any, selector: string, credential: CredentialInfo) { + return { + action: 'type', + args: [selector, encrypt(credential.value), credential.type] + }; + } + + function createWaitForLoadState() { + return { + action: 'waitForLoadState', + args: ['networkidle'] + }; + } + return workflow.map(step => { if (!step.what) return step; const newWhat: any[] = []; const processedSelectors = new Set<string>(); for (let i = 0; i < step.what.length; i++) { // ... rest of the code using the helper functions } return { ...step, what: newWhat }; }); }
1-931: Consider splitting the file into smaller, focused modules.This file has grown quite large and handles multiple distinct responsibilities (robots, runs, schedules). Consider:
- Splitting into separate route files (e.g.,
robotRoutes.ts,runRoutes.ts,scheduleRoutes.ts)- Moving business logic to separate service classes
- Implementing consistent error handling and input validation patterns across all endpoints
🧰 Tools
🪛 Biome (1.9.4)
[error] 148-148: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 150-150: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 635-635: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 639-639: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 907-907: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 913-913: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
| while (i + 1 < step.what.length && | ||
| (step.what[i + 1].action === 'type' || | ||
| step.what[i + 1].action === 'press' || | ||
| step.what[i + 1].action === 'waitForLoadState')) { | ||
| i++; | ||
| } |
There was a problem hiding this comment.
Add safeguards against potential infinite loops.
The while loops for skipping subsequent actions could potentially become infinite if the array structure is malformed.
- while (i + 1 < step.what.length &&
- (step.what[i + 1].action === 'type' ||
- step.what[i + 1].action === 'press' ||
- step.what[i + 1].action === 'waitForLoadState')) {
- i++;
- }
+ const maxIterations = step.what.length;
+ let iterations = 0;
+ while (i + 1 < step.what.length &&
+ (step.what[i + 1].action === 'type' ||
+ step.what[i + 1].action === 'press' ||
+ step.what[i + 1].action === 'waitForLoadState')) {
+ i++;
+ iterations++;
+ if (iterations > maxIterations) {
+ logger.error('Potential infinite loop detected in workflow action processing');
+ break;
+ }
+ }Also applies to: 240-245
| const updates: any = { | ||
| recording: { | ||
| ...robot.recording, | ||
| workflow | ||
| } | ||
| }; | ||
|
|
||
| await robot.save(); | ||
| if (name) { | ||
| updates.recording_meta = { | ||
| ...robot.recording_meta, | ||
| name | ||
| }; | ||
| } | ||
|
|
||
| await Robot.update(updates, { | ||
| where: { 'recording_meta.id': id } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety and validation for the update operation.
The update operation could fail if any of the spread properties are undefined. Consider adding type checking and validation.
- const updates: any = {
+ const updates: {
+ recording?: {
+ workflow: any[];
+ [key: string]: any;
+ };
+ recording_meta?: {
+ name: string;
+ [key: string]: any;
+ };
+ } = {};
+
+ // Validate recording object before spread
+ if (robot.recording) {
recording: {
- ...robot.recording,
+ ...robot.recording,
workflow
}
+ }
if (name) {
+ // Validate recording_meta object before spread
+ if (robot.recording_meta) {
updates.recording_meta = {
...robot.recording_meta,
name
};
+ }
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/routes/storage.ts (1)
166-166: Add type safety for the workflow parameter.Consider using a more specific type for the workflow parameter instead of
any[]to improve type safety and maintainability.-function handleWorkflowActions(workflow: any[], credentials: Credentials) { +interface WorkflowStep { + what?: WorkflowAction[]; + [key: string]: any; +} + +interface WorkflowAction { + action?: string; + args?: any[]; + [key: string]: any; +} + +function handleWorkflowActions(workflow: WorkflowStep[], credentials: Credentials) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/routes/storage.ts(3 hunks)
🔇 Additional comments (2)
server/src/routes/storage.ts (2)
208-213: Add safeguards against potential infinite loops.The while loops for skipping subsequent actions could potentially become infinite if the array structure is malformed.
Also applies to: 230-235
312-328: Add type safety and validation for the update operation.The update operation could fail if any of the spread properties are undefined. Consider adding type checking and validation.
|
@amhsirak Review |
1 similar comment
|
@amhsirak Review |
Closes: #434
Summary by CodeRabbit
New Features
Refactor
Bug Fixes