fix: improve script handling in execute commands for better serialization#224
fix: improve script handling in execute commands for better serialization#224goosewobbler wants to merge 96 commits intomainfrom
Conversation
…tion - Updated the `execute` function in both Electron and Tauri services to use `JSON.stringify` for string scripts, ensuring proper escaping of special characters. - Enhanced test coverage for the `execute` command to validate handling of various script formats, including strings with quotes, newlines, unicode, and backslashes. - Adjusted assertions in tests to reflect the new serialization logic, ensuring accurate expectations for script execution.
…ts plan - Deleted the `tauri-playwright-analysis.md` file to streamline documentation. - Updated the `tauri-playwright-improvements-plan.md` to include a status column for tracking improvement progress, marking the audit of JS string interpolation/escaping as complete.
Release Preview — no release
Updated automatically by ReleaseKit |
Greptile SummaryThis PR substantially refactors the
Confidence Score: 3/5Safe to merge on the happy path; two P1 edge cases around multi-statement scripts without keyword prefixes can produce silently-wrong results at runtime The vast majority of issues from the previous review round have been resolved: executeAsync is used everywhere, arguments shadowing is fixed, word-boundary checks are in place for async/function/return, and double-encoding is gone. Two genuine P1 logic gaps remain (semicolons-outside-quotes not checked in guest-js and commands.rs), lowering the score from 5. packages/tauri-plugin/guest-js/index.ts (line 234) and packages/tauri-plugin/src/commands.rs (lines 215-230) — both need a semicolon-outside-quotes scan to complement the existing keyword-prefix check
|
| Filename | Overview |
|---|---|
| packages/tauri-service/src/service.ts | patchBrowserExecute now always uses executeAsync with arrow-function callbacks and wdio_error unwrapping — the critical WebKit/macOS issues from the prior review are addressed; minor edge cases remain around multi-statement scripts without keyword prefixes |
| packages/tauri-plugin/guest-js/index.ts | Function classification uses hasTopLevelArrow and proper regex word-boundary checks; string scripts are wrapped in async IIFEs with statement keyword detection; async[\s(] check correctly avoids false positives like asyncData |
| packages/tauri-plugin/src/commands.rs | Function detection uses has_keyword_prefix (word-boundary aware), has_arrow_outside_parens (depth-tracking), and single_param_arrow; string branch has_return check uses strip_prefix+char check to avoid returnData-style false positives; string+args returns an explicit error |
| packages/tauri-service/src/commands/execute.ts | Plugin availability check uses an arrow function (safe through patchedExecute); executeWithinTauri serializes results as JSON to survive the WebDriver protocol boundary; argsJson forwarded correctly to guest-js |
| packages/electron-service/src/commands/execute.ts | wrapStringScript now produces sync IIFEs (not async) which is correct for the Electron native handler; hasSemicolonOutsideQuotes uses consecutive-backslash counting; isFunctionLike uses hasTopLevelArrow to avoid parenthesized-expression false positives |
| packages/electron-service/src/commands/executeCdp.ts | hasSemicolonOutsideQuotes updated with the same consecutive-backslash-counting fix as execute.ts; wrapStringScriptForCdp produces async function expressions (not IIFEs) suitable for Runtime.callFunctionOn with awaitPromise:true |
| packages/tauri-service/test/service.spec.ts | Tests updated to assert executeAsync is called for both function and string scripts on all providers; asserts done-callback pattern (.then() + wdio_error) and expression-return prepending |
| .github/workflows/_ci-e2e-tauri-all-providers.reusable.yml | Adds cleanup of test-runner-backend and tauri-driver processes before/after CrabNebula provider tests to prevent stale process interference |
Sequence Diagram
sequenceDiagram
participant Test as Test Code
participant TS as tauri-service/execute.ts
participant PE as patchedExecute (service.ts)
participant WD as WebDriver executeAsync
participant BW as Browser / WebKit
participant GJ as guest-js index.ts
participant RS as commands.rs (Rust)
Test->>TS: browser.tauri.execute(fn, ...args)
TS->>PE: browser.execute(executeWithinTauri, scriptStr, opts, argsJson)
PE->>PE: typeof script === function → function path
PE->>WD: originalExecuteAsync(wrappedScript, scriptStr, opts, argsJson)
WD->>BW: eval wrappedScript (with done callback as last arg)
BW->>BW: Promise.resolve(executeWithinTauri.apply(null, args_without_done)).then(arrow→done(r))
BW->>GJ: window.wdioTauri.execute(scriptStr, opts, argsJson)
GJ->>GJ: classify script (isFunctionLike?)
GJ->>GJ: build scriptToSend with Tauri API injection and __wdio_args
GJ->>RS: invoke plugin:wdio|execute { script: scriptToSend, args: [] }
RS->>RS: detect is_function / string branch
RS->>BW: window.eval(script_with_result)
BW->>BW: await user script, emit wdio-result-uuid event
RS-->>GJ: event result via Tauri event system
GJ-->>BW: resolve with result
BW->>WD: done(JSON.stringify({__wdio_value__: result}))
WD-->>PE: asyncResult
PE->>PE: check __wdio_error__, return value
PE-->>TS: ReturnValue
TS-->>Test: result
Reviews (54): Last reviewed commit: "refactor(electron-service): simplify IIF..." | Re-trigger Greptile
- Updated the `execute` function to simplify string handling by passing strings as-is, allowing Rust to manage proper escaping. - Adjusted related test cases to reflect this change, ensuring accurate expectations for string arguments passed to the plugin.
- Updated the `execute` function to improve handling of string and function scripts. - Strings are now passed as-is for embedded paths, while tauri-driver paths utilize `JSON.stringify` for proper escaping. - Adjusted comments for clarity on the handling of different script types.
- Updated the `execute` function to pass scripts as-is instead of using `JSON.stringify`, improving handling of various script formats. - Adjusted related test cases to reflect this change, ensuring accurate expectations for string arguments passed to the plugin.
- Improved error handling in the `execute` function by restructuring the parsing logic to separate error checks from the try/catch block, allowing for clearer error messages when parsing fails. - Updated related test cases to reflect the new error handling behavior, ensuring that specific error messages are thrown for parsing issues.
- Updated the `execute` function to enhance handling of string and function scripts by passing them as-is, improving clarity and functionality. - Adjusted comments for better understanding of the script processing logic, ensuring that both strings and functions are correctly wrapped for execution.
- Modified the `execute` function to wrap string scripts in an IIFE, ensuring they are callable as statement expressions. - Enhanced comments for clarity on the handling of both string and function scripts, improving understanding of the execution flow.
- Updated the `execute` function to properly wrap string scripts in an IIFE with braces, ensuring correct syntax for execution. - Adjusted related test cases to reflect the updated script format, enhancing the accuracy of expectations for script execution.
- Added a comment in the `execute` function to explain that TypeScript sends scripts as-is, necessitating serialization for proper handling. - This change enhances understanding of the script processing logic, ensuring clarity on the serialization step for arguments passed to the function.
- Updated the `execute` function to improve the handling of scripts with and without arguments. - Scripts with arguments are now wrapped in an IIFE and executed as functions, while scripts without arguments are evaluated directly. - Enhanced comments for clarity on the script evaluation process, ensuring better understanding of callable detection and execution flow.
a4e8d04 to
bf638a7
Compare
- Enhanced the `execute` function to handle embedded WebDriver scripts more effectively by passing them through untouched. - Simplified the script wrapping logic for non-embedded scripts, ensuring consistent execution behavior. - Updated related test cases to reflect the changes in script handling, improving accuracy in expectations.
- Updated the `execute` function to inject Tauri APIs as the first parameter when scripts are executed with arguments, enhancing the integration with Tauri's core functionalities. - Revised comments for clarity on the script evaluation process, ensuring a better understanding of how arguments are passed and handled. - Maintained the evaluation of script expressions for cases without arguments, preserving existing behavior.
- Updated the `execute` function to wrap scripts without arguments in an async IIFE, allowing for proper handling of statement-style scripts. - Revised comments to clarify the execution flow and ensure consistency in script handling across both with-args and no-args scenarios.
- Introduced new test cases for the `execute` function in both Electron and Tauri to cover various script types, including functions with and without arguments, statement-style strings, and async functions. - Enhanced test coverage to ensure proper handling of different script execution scenarios, improving reliability and confidence in the functionality.
…ngScript
startsWith('async') and startsWith('function') matched identifiers like
asyncData.fetchAll() and functionResult.call(), causing them to be passed
through as function-like strings instead of being wrapped in an IIFE.
Use regex with [\s(] suffix to require whitespace or ( after the keyword,
matching the Rust has_keyword_prefix approach in commands.rs.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…_return
starts_with("return") matched any identifier beginning with those letters
(returnData, returnItems, returnCode), causing expression-style scripts to
skip the return prefix and evaluate to undefined.
Use strip_prefix + check the trailing character (whitespace, ; or end of
string) to match only the actual return keyword, consistent with the
has_keyword_prefix helper used for statement keywords above.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
… function-like
startsWith('(') alone matched any parenthesized expression such as
(document.title) or (a + b), passing them through unmodified and skipping
the IIFE wrapper that adds the return statement. Align with executeCdp.ts
which already requires both startsWith('(') and includes('=>').
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
hasSemicolonOutsideQuotes in executeCdp.ts used prevChar === '\\' which
only looks one character back. A literal backslash in source ("foo\\") is
two chars \\, so the single-char check treats the character after the
escaped backslash as escaped when it is not.
Replace with the same consecutive-backslash count (odd = escaped, even =
not escaped) already used in execute.ts.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…dded providers
The bypass that sent executeWithinTauri directly to originalExecute (the
sync /execute/sync WebDriver endpoint) caused every browser.tauri.execute()
call to resolve to {} on WebKit/macOS because WKWebView does not await the
Promise returned by an async function over the sync endpoint.
Removing the bypass lets executeWithinTauri fall through to the existing
function path (lines 404-421) which already wraps in Promise.resolve().then()
and dispatches via originalExecuteAsync — the same treatment applied to all
other user-supplied function scripts.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…-embedded providers
The string branch wrapped scripts as (async function() { scriptString })
without prepending return, so expression-style calls like
browser.execute('1 + 2 + 3') or browser.execute('document.title') always
resolved to undefined on non-embedded (official/crabnebula) providers.
Apply the same statement-keyword heuristic used in the Rust plugin and
guest-js: detect statement keywords (const, let, var, if, for, while,
switch, throw, try, do, return) with a word-boundary lookahead, and prepend
return only when none are present.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Updated the `execute` function to better differentiate between function-like and plain string scripts. Function-like scripts are now wrapped in an async IIFE with a return statement, while plain strings are handled with appropriate return logic based on the presence of statement keywords. This change improves the execution of user-supplied scripts, ensuring correct behavior across different script types. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Enhanced the waitTestRunnerBackendReady function to include a settleMs parameter, allowing for a delay before resolving the promise. Updated all calls to this function within the TauriLaunchService class to utilize the new parameter, improving the initialization timing for WebSocket handlers.
Added additional cleanup commands to terminate 'test-runner-backend' and 'tauri-driver' processes before and after the CrabNebula provider tests, ensuring a cleaner test environment and preventing potential conflicts during execution.
…Driver Updated the TauriWorkerService to pass function scripts directly to originalExecute for embedded WebDriver, ensuring correct invocation with arguments. Adjusted related tests to reflect this change, enhancing clarity on how function scripts are processed.
Enhanced the `execute` function in guest-js and commands.rs to accurately identify top-level arrow functions. Introduced `hasTopLevelArrow` and `has_arrow_outside_parens` utility functions to prevent false positives in nested expressions. Updated the logic to ensure proper handling of function-like scripts, improving the execution accuracy of user-supplied scripts. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…ipt handling Introduced the `hasTopLevelArrow` function to enhance detection of top-level arrow functions in script execution. Updated the `execute` and `executeCdp` commands to utilize this new utility, ensuring more accurate identification of function-like scripts. This change improves the handling of user-supplied scripts, particularly in differentiating between function-like and plain string scripts.
Updated the `execute` function to include an additional check for scripts that start with '('. This change improves the accuracy of return detection in user-supplied scripts, ensuring better handling of various script formats.
…checks Updated the `startTestRunnerBackend` function to kill orphaned processes holding the specified port, improving resource management. Removed the `settleMs` parameter from `waitTestRunnerBackendReady` calls for cleaner code. Added a status probe for the tauri-driver on macOS to detect dead WebSocket connections, preventing hangs during execution. This change enhances the reliability and responsiveness of the Tauri service.
…n startTestRunnerBackend Enhanced the `startTestRunnerBackend` function by adding validation for the port number to ensure it falls within the valid range. Updated the process termination logic to utilize `execFileSync` for better compatibility and improved handling of orphaned processes, ensuring a more robust backend management experience.
…RunnerBackend Updated the `startTestRunnerBackend` function to use `execFileSync` for executing the PowerShell command that terminates processes occupying the specified port. This change enhances compatibility and improves the clarity of the command execution, contributing to better resource management in the backend.
…Driver Refactored the TauriWorkerService to ensure that both string and function scripts are executed using `executeAsync` for the embedded WebDriver. This change addresses the issue of WebKit not auto-awaiting Promises from synchronous executions, enhancing the reliability of script execution. Updated related tests to reflect the new execution behavior.
Updated the script execution handling in the `execute` command to remove the async IIFE wrapping for both statement and expression-style scripts. This change enhances clarity and consistency in how scripts are executed, while also updating related tests to reflect the new behavior.
| const hasStatementKeyword = /^(const|let|var|if|for|while|switch|throw|try|do|return)(?=\s|[(]|$)/.test(trimmed); | ||
| scriptToSend = hasStatementKeyword ? `(async () => { ${script} })()` : `(async () => { return ${script}; })()`; |
There was a problem hiding this comment.
Multi-statement string scripts without a keyword prefix lose the trailing statements
hasStatementKeyword only matches scripts whose first token is a control-flow keyword. A script like someFn(); anotherFn(); return result starts with someFn — no keyword match — so return is prepended, producing return someFn(); anotherFn(); return result. The anotherFn() call and the explicit return are unreachable. The electron-service already has a hasSemicolonOutsideQuotes helper that detects this correctly; the same approach should be applied here so semicolons outside string literals also set the "has statement" flag.
…ipt validation Introduced the `hasSemicolonOutsideQuotes` function to detect semicolons outside of string literals and template literals in scripts. Updated the `execute` and `executeCdp` commands to utilize this new utility, enhancing the validation of user-supplied scripts and improving overall script handling accuracy.
…d script validation Introduced the `hasSemicolonOutsideQuotes` function in both TypeScript and Rust implementations to detect semicolons outside of string literals. Updated the `execute` function to utilize this utility, improving the validation of user-supplied scripts and ensuring more accurate handling of various script formats.
…d WebDriver Enhanced the TauriWorkerService to pass string scripts directly to the synchronous execute method for the embedded WebDriver, avoiding issues with WebKit's Promise handling. Updated tests to reflect the new behavior for both string and function scripts, ensuring accurate execution and improved reliability.
No description provided.