feat: improve scrollable element detection, enhance abort error handl…#207
feat: improve scrollable element detection, enhance abort error handl…#207linked-danis wants to merge 1 commit intoalibaba:mainfrom
Conversation
…ing, and refine LLM client error parsing
|
|
|
Welcome to the community! Could you take a look at the CI error before we proceed with the review? |
There was a problem hiding this comment.
Pull request overview
This PR improves runtime robustness and performance across the agent stack by optimizing scroll-container detection, making the agent reusable after stop(), adding configurable step delays, and tightening abort/error handling and typing.
Changes:
- Optimize scrollable container discovery to avoid full-DOM scans during scroll actions.
- Improve abort handling/reusability (AbortController reset, helpers for abort + CustomEvent detail extraction).
- Add new configuration surface (
stepDelay) and harden/llms.txtfetching for invalid URLs; switch SimulatorMask visibility to CSS classes.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/page-controller/src/mask/SimulatorMask.ts | Toggle mask visibility via CSS class instead of inline display. |
| packages/page-controller/src/mask/SimulatorMask.module.css | Add .visible state styling for wrapper visibility. |
| packages/page-controller/src/dom/dom_tree/index.js | Add clarifying comments around WeakMap cache and DOM refs. |
| packages/page-controller/src/actions.ts | Add findScrollableContainer() and improve typing for scrollIntoViewIfNeeded. |
| packages/llms/src/index.ts | Refine retry/abort handling and error normalization when retrying LLM calls. |
| packages/llms/src/errors.ts | Adjust abort retryability detection logic. |
| packages/llms/src/OpenAIClient.ts | Improve HTTP error parsing and tool execution error formatting; adjust abort handling (currently has a compile-time issue). |
| packages/core/src/utils/index.ts | Make fetchLlmsTxt resilient to invalid URLs; add isAbortError and getEventDetail helpers. |
| packages/core/src/types.ts | Add stepDelay option to AgentConfig. |
| packages/core/src/PageAgentCore.ts | Use new helpers, reset AbortController on stop(), and apply configurable step delay. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Handles various forms: Error with name 'AbortError', or rawError property | ||
| */ | ||
| export function isAbortError(error: unknown): boolean { | ||
| if (error instanceof Error && error.name === 'AbortError') return true | ||
| if ( | ||
| typeof error === 'object' && | ||
| error !== null && | ||
| 'rawError' in error && | ||
| (error as { rawError?: Error }).rawError?.name === 'AbortError' | ||
| ) | ||
| return true |
There was a problem hiding this comment.
isAbortError() currently checks error.name === 'AbortError', but this repo also throws new Error('AbortError') in a few places (message is 'AbortError', name is still 'Error'). Those cases won't be detected, so aborts may be treated as real failures (and may trigger retries/logging). Consider also checking error instanceof Error && error.message === 'AbortError', and similarly for rawError when it’s an object with a name/message field rather than strictly an Error instance.
| * Handles various forms: Error with name 'AbortError', or rawError property | |
| */ | |
| export function isAbortError(error: unknown): boolean { | |
| if (error instanceof Error && error.name === 'AbortError') return true | |
| if ( | |
| typeof error === 'object' && | |
| error !== null && | |
| 'rawError' in error && | |
| (error as { rawError?: Error }).rawError?.name === 'AbortError' | |
| ) | |
| return true | |
| * Handles various forms: | |
| * - Error with name 'AbortError' | |
| * - Error with message 'AbortError' | |
| * - Plain objects with name/message 'AbortError' | |
| * - Wrapped errors via a `rawError` property | |
| */ | |
| export function isAbortError(error: unknown): boolean { | |
| // Direct Error instance: check both name and message | |
| if (error instanceof Error) { | |
| if (error.name === 'AbortError' || error.message === 'AbortError') { | |
| return true | |
| } | |
| } | |
| // Error-like objects or wrappers (including { rawError: ... }) | |
| if (typeof error === 'object' && error !== null) { | |
| const maybeError = error as { name?: unknown; message?: unknown; rawError?: unknown } | |
| // Plain object with AbortError-like fields | |
| if (maybeError.name === 'AbortError' || maybeError.message === 'AbortError') { | |
| return true | |
| } | |
| // Wrapped error under `rawError` | |
| if ('rawError' in maybeError && maybeError.rawError != null) { | |
| const raw = maybeError.rawError as unknown | |
| if (raw instanceof Error) { | |
| if (raw.name === 'AbortError' || raw.message === 'AbortError') { | |
| return true | |
| } | |
| } else if (typeof raw === 'object') { | |
| const rawObj = raw as { name?: unknown; message?: unknown } | |
| if (rawObj.name === 'AbortError' || rawObj.message === 'AbortError') { | |
| return true | |
| } | |
| } | |
| } | |
| } |
| type: 'error', | ||
| message, | ||
| rawResponse: (error as InvokeError).rawResponse, | ||
| rawResponse: error instanceof Error ? (error as InvokeError).rawResponse : undefined, |
There was a problem hiding this comment.
rawResponse is only present on InvokeError, but the current guard uses error instanceof Error and then casts to InvokeError. For plain Error instances this will always produce undefined and hides the intent. Prefer checking error instanceof InvokeError before reading .rawResponse.
| rawResponse: error instanceof Error ? (error as InvokeError).rawResponse : undefined, | |
| rawResponse: error instanceof InvokeError ? error.rawResponse : undefined, |
| const errorMessage = isAbortError ? 'Network request aborted' : 'Network request failed' | ||
| if (!isAbortError) console.error(error) |
There was a problem hiding this comment.
isAborted is computed, but the following lines still reference isAbortError, which is not defined in this scope. This will fail to compile and also breaks the intended abort/no-log behavior. Use the same variable name consistently for the abort branch and logging condition.
| const errorMessage = isAbortError ? 'Network request aborted' : 'Network request failed' | |
| if (!isAbortError) console.error(error) | |
| const errorMessage = isAborted ? 'Network request aborted' : 'Network request failed' | |
| if (!isAborted) console.error(error) |
| error instanceof InvokeError && | ||
| error.rawError instanceof Error && | ||
| error.rawError.name === 'AbortError' |
There was a problem hiding this comment.
The abort short-circuit in withRetry only triggers for InvokeError.rawError.name === 'AbortError', but LLM.invoke throws new Error('AbortError') when abortSignal.aborted (message is 'AbortError', name is still 'Error'). That means aborted requests will be logged, surfaced via onError, and retried. Consider standardizing abort exceptions to have name === 'AbortError' (e.g., DOMException) or broadening the check to also detect the existing new Error('AbortError') pattern.
| error instanceof InvokeError && | |
| error.rawError instanceof Error && | |
| error.rawError.name === 'AbortError' | |
| ( | |
| error instanceof InvokeError && | |
| error.rawError instanceof Error && | |
| (error.rawError.name === 'AbortError' || error.rawError.message === 'AbortError') | |
| ) || | |
| ( | |
| error instanceof Error && | |
| (error.name === 'AbortError' || error.message === 'AbortError') | |
| ) |
Hey! Caught a few bugs and performance hiccups while running the agent in prod. This PR groups up a bunch of fixes to make things more robust and reusable.
The main updates:
Fixes & Perf:
Fixed agent reuse: stop() wasn't resetting the AbortController, leaving the agent completely dead after stopping. It actually resets now and is fully reusable.
Optimized scroll lookups: The scroll container lookup was firing querySelectorAll('*') on every scroll (yikes). Swapped this out to check common selectors first, then viewport elements. It's noticeably faster on heavy pages now.
Configurable step delay: Step delay is no longer hardcoded to 0.4s and can be set via the new stepDelay option.
Safe LLM fetch: fetchLlmsTxt now gracefully returns null instead of outright crashing on invalid URLs.
CSS over inline styles: SimulatorMask now uses proper CSS classes instead of injecting inline display styles.
Type Safety & Error Handling:
Cleaned up error casting: Added isAbortError() and getEventDetail() helpers. We can finally check aborts properly without relying on as any type hacks.
Removed rawError nonsense: Replaced all the messy (error as any)?.rawError?.name checks with proper instanceof usage.
WebKit types: scrollIntoViewIfNeeded is now correctly typed using the WebKit extension interface (no more as any).