JS injection support #2300
JS injection support #2300
Conversation
Mzack9999
added
the
Type: Enhancement
WalkthroughAdds support for injecting and executing multiple JavaScript snippets during headless browsing. Introduces page setup/navigation with network capture, separates screenshot/body extraction and page closing, exposes ExecuteJavascriptCodesWithPage, updates ScreenshotWithBody signature to accept jsCodes, and adds a CLI flag for JavaScript snippets. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Runner
participant Browser
participant Page as "rod.Page"
participant JS as "JS Runtime"
CLI->>Runner: options (url, ..., JavascriptCodes)
Runner->>Browser: ScreenshotWithBody(url, ..., jsCodes)
Browser->>Page: setupPageAndNavigate(url, timeout, headers, jsCodes)
Page->>Browser: network events captured (requests/responses/failed)
alt jsCodes present
Browser->>Page: ExecuteJavascriptCodesWithPage(page, jsCodes)
loop per snippet
Page->>JS: Eval(snippet)
JS-->>Page: RuntimeRemoteObject (result)
end
Page-->>Browser: js results
end
Browser->>Page: takeScreenshotAndGetBody(page, idle, fullPage)
Page-->>Browser: screenshot + HTML body
Browser->>Browser: closePage(page)
Browser-->>Runner: screenshot, body, networkData, error?
Runner-->>CLI: output/results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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)
runner/headless.go (1)
279-292: Consider error handling strategy for multiple JS snippets.Currently, if one JavaScript snippet fails (line 287), all subsequent snippets are skipped. Depending on your use case, you might want to:
- Continue executing remaining snippets and collect all errors
- Add logging to indicate which snippet failed
- Add per-snippet timeouts to prevent long-running scripts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
runner/headless.go(3 hunks)runner/options.go(2 hunks)runner/runner.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
runner/runner.go (1)
common/customheader/customheader.go (1)
CustomHeaders(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Test Builds (macOS-latest)
- GitHub Check: Test Builds (windows-latest)
- GitHub Check: Test Builds (ubuntu-latest)
- GitHub Check: Analyze (go)
- GitHub Check: Functional Test (macOS-latest)
- GitHub Check: Functional Test (windows-latest)
- GitHub Check: Functional Test (ubuntu-latest)
- GitHub Check: release-test
🔇 Additional comments (5)
runner/options.go (2)
337-339: LGTM! JavaScript codes field properly added.The new
JavascriptCodesfield is well-placed and follows Go naming conventions.
410-410: LGTM! CLI flag properly configured.The
--javascript-codeflag is correctly configured withStringSliceOptionsto accept multiple JavaScript snippets.runner/runner.go (1)
71-71: LGTM! Error handling updated consistently.The migration from
errorutiltoerrkitfor error wrapping is applied consistently.Also applies to: 2517-2517
runner/headless.go (2)
113-113: LGTM! Signature properly extended.The new
jsCodesparameter is correctly added to support JavaScript injection.
195-200: Verify that ignoring ExecuteJavascriptCodesWithPage outputs is intentional.The method returns
[]*proto.RuntimeRemoteObject(line 279) but the caller discards this return value (line 196, using_). If the outputs are needed for debugging, logging, or validation, consider capturing and using them.
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
runner/headless.go (2)
302-315: Consider adding context to JS execution errors.When a JS snippet fails to execute, the error doesn't indicate which snippet in the slice caused the failure. This could make debugging difficult when multiple JS codes are provided.
Consider wrapping the error with the index or a snippet of the failing JS:
for _, js := range jsc { if js == "" { continue } output, err := page.Eval(js) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to execute JS snippet (index %d): %w", len(outputs), err) } outputs = append(outputs, output) }
302-315: Simplify method signature to return only error.The method's return value
[]*proto.RuntimeRemoteObjectis unused. The sole caller at line 209 discards it with_and only checks the error. Change the signature tofunc (b *Browser) ExecuteJavascriptCodesWithPage(page *rod.Page, jsc []string) errorand remove theoutputsslice and return statement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
runner/headless.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test Builds (macOS-latest)
- GitHub Check: Test Builds (ubuntu-latest)
- GitHub Check: Test Builds (windows-latest)
- GitHub Check: Functional Test (windows-latest)
- GitHub Check: Analyze (go)
- GitHub Check: Functional Test (macOS-latest)
- GitHub Check: Functional Test (ubuntu-latest)
🔇 Additional comments (3)
runner/headless.go (3)
208-213: JS injection timing is appropriate for the use case.Executing JavaScript after navigation but before waiting for FirstMeaningfulPaint is the correct approach for the stated objective of manipulating client-side storage (e.g., localStorage) before the page fully renders. This allows injected code to modify page state that affects rendering and behavior.
220-238: Well-structured error handling with graceful degradation.The incremental error handling is appropriate:
- Critical errors (WaitLoad, screenshot) are propagated
- Best-effort operations (WaitIdle) are attempted but not fatal
- Partial success is supported (screenshot returned even if body extraction fails at line 234)
This allows useful data capture even when some operations fail.
240-243: LGTM!Simple, focused cleanup method. Ignoring the Close error is acceptable for cleanup operations where no recovery is possible.
✅ Actions performedComments resolved. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
133-133: Documentation for the new JavaScript injection flag is clear and consistent.The flag is correctly positioned in the HEADLESS section, uses proper naming conventions, and accurately describes the feature with
string[]to indicate multiple values can be provided.Consider adding an example or cross-reference in the
# Running httpxsection to help users discover this feature. For instance, you might add a brief usage example showing how to inject a JavaScript snippet that sets localStorage entries, similar to how other complex features are illustrated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test Builds (windows-latest)
- GitHub Check: Test Builds (macOS-latest)
- GitHub Check: Test Builds (ubuntu-latest)
- GitHub Check: Analyze (go)
- GitHub Check: Functional Test (ubuntu-latest)
- GitHub Check: Functional Test (macOS-latest)
- GitHub Check: Functional Test (windows-latest)
Closes #2067
Previously implemented at #2066
Summary by CodeRabbit
New Features
Improvements