feat: change credentials based on input type #375
Conversation
WalkthroughThis pull request introduces a comprehensive update to credential and workflow management across multiple files. The changes primarily focus on enhancing the structure of credential handling, introducing more detailed type information, and refining the way credentials and workflows are processed. The modifications span several components, including interpretation logic, storage routes, workflow generation, and the robot editing interface, with a consistent approach to adding more granular type and value tracking for credentials. Changes
Suggested labels
Suggested reviewers
Poem
Possibly related PRs
🪧 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: 3
🔭 Outside diff range comments (2)
src/api/storage.ts (2)
Line range hint
1-200: Standardize error handling across API functions.The file shows inconsistent error logging patterns. Some functions use
console.logwhile others useconsole.error. Additionally, error messages could be more specific about what failed.Consider:
- Using
console.errorconsistently for error logging- Adding error codes for different types of failures
- Creating a centralized error handling utility
Example implementation:
type ApiError = { code: string; message: string; details?: unknown; }; const handleApiError = (error: unknown, context: string): null | false => { const apiError: ApiError = { code: error instanceof Error ? 'UNKNOWN_ERROR' : 'INVALID_ERROR_TYPE', message: error instanceof Error ? error.message : 'Unknown error occurred', details: error }; console.error(`${context}:`, apiError); return null; // or false based on the function's return type };
Line range hint
1-200: Improve type safety across API functions.The file has several areas where type safety could be enhanced:
anytype is used in error handling- Inconsistent Promise return type declarations
- Missing type definitions for API responses
Consider:
- Creating proper error types
- Consistently using explicit Promise return types
- Defining interfaces for API responses
Example:
interface ApiResponse<T> { data: T; status: number; message?: string; } // Use in functions: export const getStoredRecording = async (id: string): Promise<ApiResponse<Recording> | null> => { try { const response = await axios.get<ApiResponse<Recording>>(`${apiUrl}/storage/recordings/${id}`); return response.data; } catch (error: Error) { console.error(`Failed to retrieve recording ${id}:`, error.message); return null; } };
🧹 Nitpick comments (4)
src/components/robot/RobotEdit.tsx (1)
395-397: Remove Redundant FragmentThe
Fragment(<>...</>) wrapping{renderAllCredentialFields()}is unnecessary since it contains only a single child. Removing theFragmentsimplifies the JSX structure.Apply this diff to remove the redundant
Fragment:-{<> {renderAllCredentialFields()} -</>} +{renderAllCredentialFields()}🧰 Tools
🪛 Biome (1.9.4)
[error] 395-397: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
server/src/workflow-management/classes/Generator.ts (1)
Line range hint
999-1058: Enhanced workflow optimization with input type support.The optimization logic has been updated to include input type information when converting press actions to type actions. However, there's some code duplication in the initialization of the input object.
Consider extracting the input object initialization into a constant or helper function:
+const createEmptyInput = () => ({ selector: '', value: '', type: '', actionCounter: 0 }); + private optimizeWorkflow = (workflow: WorkflowFile) => { - let input = { - selector: '', - value: '', - type: '', - actionCounter: 0, - }; + let input = createEmptyInput(); // ... rest of the code ... - input = { selector: '', value: '', type: '', actionCounter: 0 }; + input = createEmptyInput();src/api/storage.ts (2)
8-11: Add type constraints and documentation for theCredentialInfointerface.Consider using a string literal union type for the
typefield to enforce valid credential types and add JSDoc comments explaining the interface usage.+/** + * Represents credential information with its value and type. + * @property value - The credential value + * @property type - The type of credential (e.g., 'password', 'apiKey', 'token') + */ interface CredentialInfo { value: string; - type: string; + type: 'password' | 'apiKey' | 'token' | string; }
Line range hint
31-43: Enhance error handling for credential updates.The current error handling doesn't distinguish between credential-specific errors and other update failures. Consider adding specific error handling for credential validation failures.
export const updateRecording = async (id: string, data: { name?: string; limit?: number, credentials?: Credentials }): Promise<boolean> => { try { const response = await axios.put(`${apiUrl}/storage/recordings/${id}`, data); if (response.status === 200) { return true; } else { throw new Error(`Couldn't update recording with id ${id}`); } } catch (error: any) { + if (error.response?.data?.code === 'INVALID_CREDENTIALS') { + console.error(`Invalid credentials provided: ${error.response.data.message}`); + } console.error(`Error updating recording: ${error.message}`); return false; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
maxun-core/src/interpret.ts(1 hunks)server/src/routes/storage.ts(1 hunks)server/src/workflow-management/classes/Generator.ts(5 hunks)src/api/storage.ts(1 hunks)src/components/robot/RobotEdit.tsx(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/robot/RobotEdit.tsx
[error] 156-156: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 395-397: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (4)
src/components/robot/RobotEdit.tsx (1)
192-192: 🛠️ Refactor suggestionHandle Unicode Characters Correctly When Splitting Strings
Using
split('')on a string may not handle Unicode characters properly, especially for characters represented by surrogate pairs. To correctly handle all Unicode characters, use the spread operator[...string]orArray.from(string).Apply this diff to update the code:
-const chars = credentialInfo.value.split(''); +const chars = [...credentialInfo.value];Likely invalid or redundant comment.
maxun-core/src/interpret.ts (1)
493-499: LGTM! Good defensive programming practice.The conditional block ensures that only the first two arguments are passed to 'press' and 'type' methods, which helps prevent potential issues with credential handling.
server/src/workflow-management/classes/Generator.ts (2)
477-480: Good addition of input type detection.The code now retrieves the input type from element attributes, which improves type-awareness for credential handling. This enhancement aligns well with the PR's objective of handling credentials based on input type.
488-488: Verify the encryption of sensitive data.The code passes the encrypted key along with the input type. Ensure that sensitive data is properly encrypted before transmission.
✅ Verification successful
Encryption implementation verified and secure
The encryption of sensitive data is properly implemented using AES-256-CBC with secure IV handling and proper key management. The codebase consistently applies encryption for sensitive data and includes proper error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper encryption usage rg -A 5 "encrypt\(" --type tsLength of output: 2250
Script:
#!/bin/bash # Find the encrypt function implementation ast-grep --pattern 'function encrypt($_) { $$$ }' # Also search for any files that might contain the encrypt implementation rg -l "encrypt" --type tsLength of output: 266
Script:
#!/bin/bash # Check auth.ts content cat server/src/utils/auth.ts # Try different patterns for function declaration ast-grep --pattern 'const encrypt = $_' # Check how interpreter handles encrypted data rg -A 5 "decrypt|encrypted" --type tsLength of output: 13485
src/components/robot/RobotEdit.tsx
Outdated
| const character: string = action.args[1]; | ||
| const inputType: string = action.args[2] || ''; | ||
|
|
||
| if (!credentials.hasOwnProperty(currentSelector)) { |
There was a problem hiding this comment.
Use Object.hasOwn() Instead of hasOwnProperty
To ensure proper object property checks and avoid issues with objects that do not inherit from Object.prototype, it's recommended to use Object.hasOwn(credentials, currentSelector) instead of credentials.hasOwnProperty(currentSelector).
Apply this diff to fix the issue:
-if (!credentials.hasOwnProperty(currentSelector)) {
+if (!Object.hasOwn(credentials, currentSelector)) {📝 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.hasOwnProperty(currentSelector)) { | |
| if (!Object.hasOwn(credentials, currentSelector)) { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 156-156: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
| if (clickIndex !== -1) { | ||
| const chars = credential.split(''); | ||
| const chars = credentialInfo.value.split(''); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Unicode-Safe String Splitting
When splitting strings that may contain Unicode characters, split('') may not handle surrogate pairs correctly. Use the spread operator [...string] or Array.from(string) to ensure all characters are handled properly.
Apply this diff to update the code:
-const chars = credentialInfo.value.split('');
+const chars = [...credentialInfo.value];Committable suggestion skipped: line range outside the PR's diff.
| args: [selector, encrypt(char)] | ||
| args: [ | ||
| selector, | ||
| encrypt(char), |
There was a problem hiding this comment.
Ensure Asynchronous Handling of encrypt Function
The encrypt function might be asynchronous, similar to how decrypt is used with await in decryptWorkflowActions. If encrypt returns a Promise, you need to handle it with await and modify the function accordingly.
Apply this diff to handle encrypt asynchronously:
-function updateTypeActionsInWorkflow(workflow: any[], credentials: Credentials) {
+async function updateTypeActionsInWorkflow(workflow: any[], credentials: Credentials) {
return await Promise.all(workflow.map(async step => {
if (!step.what) return step;
const indicesToRemove = new Set<number>();
step.what.forEach((action: any, index: number) => {
// existing logic...
});
const filteredWhat = step.what.filter((_: any, index: number) => !indicesToRemove.has(index));
await Promise.all(Object.entries(credentials).map(async ([selector, credentialInfo]) => {
const clickIndex = filteredWhat.findIndex((action: any) =>
action.action === 'click' && action.args?.[0] === selector
);
if (clickIndex !== -1) {
const chars = [...credentialInfo.value];
await Promise.all(chars.map(async (char, i) => {
filteredWhat.splice(clickIndex + 1 + (i * 2), 0, {
action: 'type',
args: [
selector,
- encrypt(char),
+ await encrypt(char),
credentialInfo.type
]
});
filteredWhat.splice(clickIndex + 2 + (i * 2), 0, {
action: 'waitForLoadState',
args: ['networkidle']
});
}));
}
}));
return {
...step,
what: filteredWhat
};
}));
}Ensure that all calls to updateTypeActionsInWorkflow handle the Promise returned due to the function being asynchronous.
📝 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.
| encrypt(char), | |
| async function updateTypeActionsInWorkflow(workflow: any[], credentials: Credentials) { | |
| return await Promise.all(workflow.map(async step => { | |
| if (!step.what) return step; | |
| const indicesToRemove = new Set<number>(); | |
| step.what.forEach((action: any, index: number) => { | |
| // existing logic... | |
| }); | |
| const filteredWhat = step.what.filter((_: any, index: number) => !indicesToRemove.has(index)); | |
| await Promise.all(Object.entries(credentials).map(async ([selector, credentialInfo]) => { | |
| const clickIndex = filteredWhat.findIndex((action: any) => | |
| action.action === 'click' && action.args?.[0] === selector | |
| ); | |
| if (clickIndex !== -1) { | |
| const chars = [...credentialInfo.value]; | |
| await Promise.all(chars.map(async (char, i) => { | |
| filteredWhat.splice(clickIndex + 1 + (i * 2), 0, { | |
| action: 'type', | |
| args: [ | |
| selector, | |
| await encrypt(char), | |
| credentialInfo.type | |
| ] | |
| }); | |
| filteredWhat.splice(clickIndex + 2 + (i * 2), 0, { | |
| action: 'waitForLoadState', | |
| args: ['networkidle'] | |
| }); | |
| })); | |
| } | |
| })); | |
| return { | |
| ...step, | |
| what: filteredWhat | |
| }; | |
| })); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/components/robot/RobotEdit.tsx (6)
Line range hint
11-20: Consider using a more specific type forparamsinRobotMeta.The
paramsproperty usesany[]which reduces type safety. Consider defining a more specific type based on the expected parameter structure.interface RobotMeta { name: string; id: string; createdAt: string; pairs: number; updatedAt: string; - params: any[]; + params: Array<{ + name: string; + value: unknown; + }>; }
59-62: Add JSDoc comments to document theCredentialInfointerface.Adding documentation will help other developers understand the purpose and usage of this interface.
+/** + * Represents credential information with its value and type. + * @property value - The actual credential value + * @property type - The type of credential (e.g., 'password', 'text') + */ interface CredentialInfo { value: string; type: string; }
92-94: Enhance email pattern validation.The current email validation is overly simplistic. Consider using a more robust regex pattern or a validation library.
const isEmailPattern = (value: string): boolean => { - return value.includes('@'); + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + return emailRegex.test(value); };
102-120: Extract credential type constants for better maintainability.Consider extracting the credential types into constants to avoid magic strings and improve maintainability.
+const CREDENTIAL_TYPES = { + PASSWORD: 'password', + EMAIL: 'email', + USERNAME: 'username', + OTHER: 'other', +} as const; -const determineCredentialType = (selector: string, info: CredentialInfo): 'password' | 'email' | 'username' | 'other' => { +const determineCredentialType = (selector: string, info: CredentialInfo): typeof CREDENTIAL_TYPES[keyof typeof CREDENTIAL_TYPES] => { if (info.type === 'password') { - return 'password'; + return CREDENTIAL_TYPES.PASSWORD; } if (isEmailPattern(info.value) || selector.toLowerCase().includes('email')) { - return 'email'; + return CREDENTIAL_TYPES.EMAIL; } if (isUsernameSelector(selector)) { - return 'username'; + return CREDENTIAL_TYPES.USERNAME; } - return 'other'; + return CREDENTIAL_TYPES.OTHER; };
428-432: Remove unnecessary fragment.The fragment wrapper is redundant as it contains only one child element.
-{(robot.isLogin || Object.keys(credentials).length > 0) && ( - <> - {renderAllCredentialFields()} - </> -)} +{(robot.isLogin || Object.keys(credentials).length > 0) && renderAllCredentialFields()}🧰 Tools
🪛 Biome (1.9.4)
[error] 429-431: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
354-364: Enhance error handling in credential payload preparation.The credential payload preparation could benefit from additional validation and error handling.
const credentialsForPayload = Object.entries(credentials).reduce((acc, [selector, info]) => { + if (!info || typeof info.value === 'undefined') { + console.warn(`Invalid credential info for selector: ${selector}`); + return acc; + } const enforceType = info.type === 'password' ? 'password' : 'text'; acc[selector] = { value: info.value, type: enforceType }; return acc; }, {} as Record<string, CredentialInfo>);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/robot/RobotEdit.tsx(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/robot/RobotEdit.tsx
[error] 429-431: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
| const extractInitialCredentials = (workflow: any[]): Credentials => { | ||
| const credentials: Credentials = {}; | ||
|
|
||
| // Helper function to check if a character is printable | ||
| const isPrintableCharacter = (char: string): boolean => { | ||
| return char.length === 1 && !!char.match(/^[\x20-\x7E]$/); | ||
| }; | ||
|
|
||
|
|
||
| // Process each step in the workflow | ||
| workflow.forEach(step => { | ||
| if (!step.what) return; | ||
|
|
||
|
|
||
| // Keep track of the current input field being processed | ||
| let currentSelector = ''; | ||
| let currentValue = ''; | ||
| let currentType = ''; | ||
|
|
||
| // Process actions in sequence to maintain correct text state | ||
| step.what.forEach((action: any) => { | ||
| if ( | ||
| (action.action === 'type' || action.action === 'press') && | ||
| action.args?.length >= 2 && | ||
| typeof action.args[1] === 'string' | ||
| ) { | ||
| let currentSelector: string = action.args[0]; | ||
| let character: string = action.args[1]; | ||
|
|
||
| if (!credentials.hasOwnProperty(currentSelector)) { | ||
| credentials[currentSelector] = ''; | ||
| const selector: string = action.args[0]; | ||
| const character: string = action.args[1]; | ||
| const inputType: string = action.args[2] || ''; | ||
|
|
||
| // If we're dealing with a new selector, store the previous one | ||
| if (currentSelector && selector !== currentSelector) { | ||
| if (!credentials[currentSelector]) { | ||
| credentials[currentSelector] = { | ||
| value: currentValue, | ||
| type: currentType | ||
| }; | ||
| } else { | ||
| credentials[currentSelector].value = currentValue; | ||
| } | ||
| } | ||
|
|
||
| if (isPrintableCharacter(character)) { | ||
| credentials[currentSelector] += character; | ||
|
|
||
| // Update current tracking variables | ||
| if (selector !== currentSelector) { | ||
| currentSelector = selector; | ||
| currentValue = credentials[selector]?.value || ''; | ||
| currentType = inputType || credentials[selector]?.type || ''; | ||
| } | ||
|
|
||
| // Handle different types of key actions | ||
| if (character === 'Backspace') { | ||
| // Remove the last character when backspace is pressed | ||
| currentValue = currentValue.slice(0, -1); | ||
| } else if (isPrintableCharacter(character)) { | ||
| // Add the character to the current value | ||
| currentValue += character; | ||
| } | ||
| // Note: We ignore other special keys like 'Shift', 'Enter', etc. | ||
| } | ||
| }); | ||
|
|
||
| // Store the final state of the last processed selector | ||
| if (currentSelector) { | ||
| credentials[currentSelector] = { | ||
| value: currentValue, | ||
| type: currentType | ||
| }; | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor extractInitialCredentials for better maintainability.
The function is quite long and handles multiple responsibilities. Consider breaking it down into smaller, focused functions.
+const processKeyAction = (
+ character: string,
+ currentValue: string,
+ isPrintableCharacter: (char: string) => boolean
+): string => {
+ if (character === 'Backspace') {
+ return currentValue.slice(0, -1);
+ }
+ if (isPrintableCharacter(character)) {
+ return currentValue + character;
+ }
+ return currentValue;
+};
+const processWorkflowStep = (
+ step: any,
+ credentials: Credentials
+): Credentials => {
+ if (!step.what) return credentials;
+
+ let currentSelector = '';
+ let currentValue = '';
+ let currentType = '';
+
+ // ... rest of the step processing logic
+
+ return credentials;
+};
const extractInitialCredentials = (workflow: any[]): Credentials => {
const credentials: Credentials = {};
workflow.forEach(step => {
- // Current implementation
+ processWorkflowStep(step, credentials);
});
return credentials;
};Also, consider adding error handling for malformed workflow data:
const extractInitialCredentials = (workflow: any[]): Credentials => {
+ if (!Array.isArray(workflow)) {
+ console.error('Invalid workflow data');
+ return {};
+ }
const credentials: Credentials = {};
// ... rest of the implementation
};Committable suggestion skipped: line range outside the PR's diff.
amhsirak
changed the title
2 participants
Display and change credentials or user inputs based on input type
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes