fix: skip click action if selector not visible #267
Conversation
WalkthroughThe pull request introduces enhancements to error handling in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
🔭 Outside diff range comments (1)
maxun-core/src/interpret.ts (1)
Line range hint
476-484: Enhance executeAction function with better error handling and loggingThe current implementation of
executeActioncould benefit from more robust error handling and consistent logging.Consider this improved implementation:
const executeAction = async (invokee: any, methodName: string, args: any) => { - console.log("Executing action:", methodName, args); + this.log(`Executing action: ${methodName} with args: ${JSON.stringify(args)}`, Level.LOG); + try { if (!args || Array.isArray(args)) { await (<any>invokee[methodName])(...(args ?? [])); } else { await (<any>invokee[methodName])(args); } + } catch (error) { + this.log(`Action execution failed: ${methodName}, Error: ${error.message}`, Level.ERROR); + throw error; // Re-throw to be handled by the specific action handlers + } };This change:
- Uses the class logger instead of console.log
- Adds consistent error handling
- Improves logging with better context
🧹 Nitpick comments (1)
maxun-core/src/interpret.ts (1)
Line range hint
1-1024: General improvements needed across the codebase
Replace console.log statements with the class logger for consistency:
- Line 477:
console.log("Executing action:"...)- Line 673:
console.log("MATCHED ACTION:"...)- Line 674:
console.log("MATCHED ACTION ID:"...)- And others...
Standardize error handling patterns across methods:
- Some methods use try-catch with logging
- Others silently catch errors
- Some re-throw errors while others don't
Consider creating a centralized error handling utility:
private handleError(error: Error, context: string, level: Level = Level.ERROR): void { this.log(`${context}: ${error.message}`, level); if (this.options.debug) { this.log(error.stack || 'No stack trace available', Level.DEBUG); } }This would provide consistent error handling and logging across the codebase.
| try{ | ||
| await executeAction(invokee, methodName, [step.args[0], { force: true }]); | ||
| } catch (error) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Improve error handling for click actions
While the attempt to handle click failures is good, the current implementation has some issues:
- Silent failure: The error is caught but not logged, making it difficult to debug issues.
- Potentially dangerous: Force clicking without checking element state could lead to unintended interactions.
- Inconsistent indentation in the try-catch block.
Consider this improved implementation:
try {
await executeAction(invokee, methodName, step.args);
} catch (error) {
- try{
- await executeAction(invokee, methodName, [step.args[0], { force: true }]);
- } catch (error) {
- continue
- }
+ try {
+ // Check if element exists but is not visible
+ const selector = step.args[0];
+ const isPresent = await page.$(selector);
+ if (!isPresent) {
+ this.log(`Element not found: ${selector}`, Level.ERROR);
+ continue;
+ }
+ const isVisible = await page.isVisible(selector);
+ if (!isVisible) {
+ this.log(`Element not visible: ${selector}`, Level.WARN);
+ }
+ await executeAction(invokee, methodName, [selector, { force: true }]);
+ } catch (error) {
+ this.log(`Click action failed: ${error.message}`, Level.ERROR);
+ continue;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
Popups visible only for a certain duration on the page can not be clicked on. If click and force click actions fail then continue as the selector is not visible.
Summary by CodeRabbit
clickaction with a retry mechanism.getSelectorsmethod to return an empty array for empty workflows.