454 bug worker screenshot only captures html without cssjs#457
Open
mizhm wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe PR adds an ChangesAgent-browser worker integration
Sequence Diagram(s)sequenceDiagram
participant processJob
participant BrowserSession
participant TakeScreenshotBase64
participant agentBrowserCLI as "agent-browser CLI"
processJob->>BrowserSession: NewBrowserSession(sessionID, toolPath)
processJob->>TakeScreenshotBase64: TakeScreenshotBase64(ctx, browser, rawURL)
TakeScreenshotBase64->>BrowserSession: TakeScreenshot(ctx, url, opts)
BrowserSession->>agentBrowserCLI: set, open, screenshot --json -
agentBrowserCLI-->>BrowserSession: screenshot file path in JSON
BrowserSession-->>TakeScreenshotBase64: base64 image data
processJob->>BrowserSession: Close()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
worker/Dockerfile (1)
28-36: 🩺 Stability & Availability | 🟠 MajorAdd missing system dependencies for Chrome browser runtime
The current Dockerfile installs
ca-certificates,curl, anddnsutilsbut lacks the essential shared libraries required to run the Chrome browser installed byagent-browseron Debian bookworm-slim (e.g.,libnss3,libatk1.0-0,libx11-6). Without these, the browser binary will fail to launch, causing screenshots to fail.Update the
apt-get installline inworker/Dockerfileto include the necessary dependencies:RUN apt-get update && apt-get install -y --no-install-recommends \ ca-certificates \ bash \ libpcap0.8 \ curl \ iputils-ping \ dnsutils \ libasound2 \ libatk1.0-0 \ libatk-bridge2.0-0 \ libatspi2.0-0 \ libcairo2 \ libcups2 \ libdbus-1-3 \ libexpat1 \ libfontconfig1 \ libgbm1 \ libglib2.0-0 \ libnspr4 \ libnss3 \ libpango-1.0-0 \ libpangocairo-1.0-0 \ libx11-6 \ libx11-xcb1 \ libxcb1 \ libxcomposite1 \ libxcursor1 \ libxdamage1 \ libxext6 \ libxfixes3 \ libxi6 \ libxrandr2 \ libxrender1 \ libxss1 \ libxtst6 \ && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@worker/Dockerfile` around lines 28 - 36, The Dockerfile’s base runtime image is missing shared libraries required by the Chrome browser bundled with agent-browser, so screenshots and browser launch can fail. Update the apt-get install block in worker/Dockerfile to include the full set of Chrome runtime dependencies (for example the libnss3, libatk1.0-0, libx11-6, and related X11/GTK libraries) alongside the existing packages. Keep the change localized to the install step so the worker image can run the browser binary successfully.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@worker/internal/worker/browser.go`:
- Around line 100-103: The timeout check in the browser load path is using the
wrong context after b.run(waitCtx, "wait", "--load", "load") fails. Update the
error handling in the browser method that performs the load wait to inspect
waitCtx.Err() instead of ctx.Err(), so load-timeout failures are classified
correctly and still return the timeout loading page %s error when the wait
context is exceeded.
- Around line 56-63: ensureAgentBrowserChrome currently calls b.run with
context.Background() for both the "--version" check and the "install" step,
which can block startup forever if agent-browser hangs. Update
ensureAgentBrowserChrome to create bounded contexts with explicit timeouts for
both commands, pass those contexts into b.run, and make sure the contexts are
canceled after each use so the startup path can fail fast instead of waiting
indefinitely.
- Around line 167-175: Transient retry detection in browser execution is only
inspecting stdout from cmd.Output(), so stderr-only transient failures are
missed. Update the retry block in the browser command execution path to combine
stdout with stderr from the returned exec.ExitError before calling
isTransientError, keeping the existing retry/sleep logic in place. Use the
cmd.Output() call and the isTransientError check as the key points to adjust.
In `@worker/internal/worker/client.go`:
- Around line 165-173: The browser startup path in NewBrowserSession/initSession
is not cancellable because ensureAgentBrowserChrome() and Close() use
context.Background(), so shutdown can be delayed while stateMu is held. Update
the initSession flow to thread sessionCtx (or another ctx derived from the
worker ctx) through ensureAgentBrowserChrome and Close, or move the blocking
setup/cleanup outside the locked section so cancellation can interrupt it. Use
the NewBrowserSession, ensureAgentBrowserChrome, and Close call sites in the
worker startup path to make the change.
In `@worker/internal/worker/job.go`:
- Around line 51-53: The screenshot job flow in the worker currently ignores
BrowserSession cleanup failures, which can hide leaks across repeated jobs.
Update the per-job browser handling around NewBrowserSession and
TakeScreenshotBase64 to capture and log any error returned by jobBrowser.Close()
at minimum, and if the browser session supports a context-aware or bounded close
path, use that instead so cleanup failures are visible and handled.
- Around line 51-64: The screenshot result is storing the raw input URL instead
of the normalized URL used for capture. In the job handling flow in job.go,
normalize the URL once with the same logic used by
TakeScreenshotBase64/formatURL, then pass that normalized value into both the
screenshot call and resultData.URL so the payload reflects the actual captured
URL.
In `@worker/internal/worker/screenshot.go`:
- Around line 51-60: The screenshot capture path in TakeScreenshot is swallowing
failures from browser.TakeScreenshot, which lets the caller continue as if the
capture succeeded. Update TakeScreenshot to return the error immediately after
logging it, and make sure the timeout-specific and general log messages stay in
the error path; this will allow job.go to receive the failure and send the
intended error payload instead of an empty screenshot.
---
Outside diff comments:
In `@worker/Dockerfile`:
- Around line 28-36: The Dockerfile’s base runtime image is missing shared
libraries required by the Chrome browser bundled with agent-browser, so
screenshots and browser launch can fail. Update the apt-get install block in
worker/Dockerfile to include the full set of Chrome runtime dependencies (for
example the libnss3, libatk1.0-0, libx11-6, and related X11/GTK libraries)
alongside the existing packages. Keep the change localized to the install step
so the worker image can run the browser binary successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e94f3cd-9615-4dbe-9b2d-872522965f8c
⛔ Files ignored due to path filters (5)
core-api/public/archived/linux_amd64/agent-browser-v0.29.zipis excluded by!**/*.zipcore-api/public/archived/linux_arm64/agent-browser-v0.29.zipis excluded by!**/*.zipcore-api/public/archived/macos_amd64/agent-browser-v0.29.zipis excluded by!**/*.zipcore-api/public/archived/windows_amd64/agent-browser-v0.29.zipis excluded by!**/*.zipworker/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
.agents/skills/agent-browser/SKILL.mdskills-lock.jsonworker/Dockerfileworker/go.modworker/internal/worker/browser.goworker/internal/worker/client.goworker/internal/worker/job.goworker/internal/worker/screenshot.go
💤 Files with no reviewable changes (1)
- worker/go.mod
Comment on lines
+56
to
+63
| if err := b.run(context.Background(), "--version"); err != nil { | ||
| return fmt.Errorf("agent-browser binary not found or not working: %w", err) | ||
| } | ||
|
|
||
| // Install Chrome | ||
| browserLog.Info("Installing Chrome via agent-browser...") | ||
| if err := b.run(context.Background(), "install"); err != nil { | ||
| return fmt.Errorf("agent-browser install failed: %w", err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add bounded timeouts for setup commands.
ensureAgentBrowserChrome runs external commands with context.Background(), so a stuck agent-browser --version/install can block startup indefinitely. Use explicit timeouts for both calls.
Suggested fix
func (b *BrowserSession) ensureAgentBrowserChrome() error {
browserLog.Info("Checking agent-browser Chrome installation...")
- // Try to run agent-browser to check if it works
- if err := b.run(context.Background(), "--version"); err != nil {
+ checkCtx, checkCancel := context.WithTimeout(context.Background(), 15*time.Second)
+ defer checkCancel()
+ if err := b.run(checkCtx, "--version"); err != nil {
return fmt.Errorf("agent-browser binary not found or not working: %w", err)
}
// Install Chrome
browserLog.Info("Installing Chrome via agent-browser...")
- if err := b.run(context.Background(), "install"); err != nil {
+ installCtx, installCancel := context.WithTimeout(context.Background(), 5*time.Minute)
+ defer installCancel()
+ if err := b.run(installCtx, "install"); err != nil {
return fmt.Errorf("agent-browser install failed: %w", err)
}📝 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.
Suggested change
| if err := b.run(context.Background(), "--version"); err != nil { | |
| return fmt.Errorf("agent-browser binary not found or not working: %w", err) | |
| } | |
| // Install Chrome | |
| browserLog.Info("Installing Chrome via agent-browser...") | |
| if err := b.run(context.Background(), "install"); err != nil { | |
| return fmt.Errorf("agent-browser install failed: %w", err) | |
| checkCtx, checkCancel := context.WithTimeout(context.Background(), 15*time.Second) | |
| defer checkCancel() | |
| if err := b.run(checkCtx, "--version"); err != nil { | |
| return fmt.Errorf("agent-browser binary not found or not working: %w", err) | |
| } | |
| // Install Chrome | |
| browserLog.Info("Installing Chrome via agent-browser...") | |
| installCtx, installCancel := context.WithTimeout(context.Background(), 5*time.Minute) | |
| defer installCancel() | |
| if err := b.run(installCtx, "install"); err != nil { | |
| return fmt.Errorf("agent-browser install failed: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@worker/internal/worker/browser.go` around lines 56 - 63,
ensureAgentBrowserChrome currently calls b.run with context.Background() for
both the "--version" check and the "install" step, which can block startup
forever if agent-browser hangs. Update ensureAgentBrowserChrome to create
bounded contexts with explicit timeouts for both commands, pass those contexts
into b.run, and make sure the contexts are canceled after each use so the
startup path can fail fast instead of waiting indefinitely.
Comment on lines
+100
to
+103
| if err := b.run(waitCtx, "wait", "--load", "load"); err != nil { | ||
| if ctx.Err() == context.DeadlineExceeded { | ||
| return "", fmt.Errorf("timeout loading page %s", url) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Timeout check uses the wrong context.
After b.run(waitCtx, ...) fails, this checks ctx.Err() instead of waitCtx.Err(), so load-timeout errors are often misclassified.
Suggested fix
- if err := b.run(waitCtx, "wait", "--load", "load"); err != nil {
- if ctx.Err() == context.DeadlineExceeded {
+ if err := b.run(waitCtx, "wait", "--load", "load"); err != nil {
+ if waitCtx.Err() == context.DeadlineExceeded {
return "", fmt.Errorf("timeout loading page %s", url)
}
return "", fmt.Errorf("wait load: %w", err)
}📝 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.
Suggested change
| if err := b.run(waitCtx, "wait", "--load", "load"); err != nil { | |
| if ctx.Err() == context.DeadlineExceeded { | |
| return "", fmt.Errorf("timeout loading page %s", url) | |
| } | |
| if err := b.run(waitCtx, "wait", "--load", "load"); err != nil { | |
| if waitCtx.Err() == context.DeadlineExceeded { | |
| return "", fmt.Errorf("timeout loading page %s", url) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@worker/internal/worker/browser.go` around lines 100 - 103, The timeout check
in the browser load path is using the wrong context after b.run(waitCtx, "wait",
"--load", "load") fails. Update the error handling in the browser method that
performs the load wait to inspect waitCtx.Err() instead of ctx.Err(), so
load-timeout failures are classified correctly and still return the timeout
loading page %s error when the wait context is exceeded.
Comment on lines
+167
to
+175
| for attempt := range maxRetries { | ||
| cmd := exec.CommandContext(ctx, binaryPath, fullArgs...) | ||
| output, err := cmd.Output() | ||
| if err == nil { | ||
| return string(output), nil | ||
| } | ||
| out := string(output) | ||
| if attempt < maxRetries-1 && isTransientError(out) { | ||
| time.Sleep(time.Duration(attempt+1) * 500 * time.Millisecond) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
Transient retry logic ignores stderr output
cmd.Output() returns only stdout to the output variable; stderr is attached to the returned error (as *exec.ExitError.Stderr). Consequently, the transient check isTransientError(out) misses errors sent to stderr, such as "Resource temporarily unavailable", preventing valid retries.
Combine stdout and stderr before checking for transient errors:
Proposed Fix
output, err := cmd.Output()
if err == nil {
return string(output), nil
}
out := string(output)
if exitErr, ok := err.(*exec.ExitError); ok && len(exitErr.Stderr) > 0 {
out = out + string(exitErr.Stderr)
}
if attempt < maxRetries-1 && isTransientError(out) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@worker/internal/worker/browser.go` around lines 167 - 175, Transient retry
detection in browser execution is only inspecting stdout from cmd.Output(), so
stderr-only transient failures are missed. Update the retry block in the browser
command execution path to combine stdout with stderr from the returned
exec.ExitError before calling isTransientError, keeping the existing retry/sleep
logic in place. Use the cmd.Output() call and the isTransientError check as the
key points to adjust.
Comment on lines
+165
to
+173
| // Initialize browser after tools are downloaded | ||
| initSession := NewBrowserSession("init", toolPath) | ||
| if err := initSession.ensureAgentBrowserChrome(); err != nil { | ||
| sysLog.ErrorE("Failed to setup agent-browser", err) | ||
| _ = initSession.Close() | ||
| stateMu.Unlock() | ||
| continue | ||
| } | ||
| _ = initSession.Close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make browser setup cancellable during shutdown.
ensureAgentBrowserChrome() and Close() run through context.Background(), so this new startup path can keep running after ctx is canceled and delay shutdown/reconnect while stateMu is held. Thread sessionCtx through setup/close, or move the blocking setup outside the locked section with cancellation support.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@worker/internal/worker/client.go` around lines 165 - 173, The browser startup
path in NewBrowserSession/initSession is not cancellable because
ensureAgentBrowserChrome() and Close() use context.Background(), so shutdown can
be delayed while stateMu is held. Update the initSession flow to thread
sessionCtx (or another ctx derived from the worker ctx) through
ensureAgentBrowserChrome and Close, or move the blocking setup/cleanup outside
the locked section so cancellation can interrupt it. Use the NewBrowserSession,
ensureAgentBrowserChrome, and Close call sites in the worker startup path to
make the change.
Comment on lines
+51
to
+53
| jobBrowser := NewBrowserSession("screenshot-"+job.Id, toolPath) | ||
| base64Image, err := TakeScreenshotBase64(ctx, jobBrowser, url) | ||
| _ = jobBrowser.Close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not discard per-job browser close failures.
With one BrowserSession per screenshot job, ignoring Close() failures hides cleanup leaks across repeated jobs. Log the close error at minimum, and prefer a bounded/context-aware close path if available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@worker/internal/worker/job.go` around lines 51 - 53, The screenshot job flow
in the worker currently ignores BrowserSession cleanup failures, which can hide
leaks across repeated jobs. Update the per-job browser handling around
NewBrowserSession and TakeScreenshotBase64 to capture and log any error returned
by jobBrowser.Close() at minimum, and if the browser session supports a
context-aware or bounded close path, use that instead so cleanup failures are
visible and handled.
Comment on lines
+51
to
+64
| jobBrowser := NewBrowserSession("screenshot-"+job.Id, toolPath) | ||
| base64Image, err := TakeScreenshotBase64(ctx, jobBrowser, url) | ||
| _ = jobBrowser.Close() | ||
| if err != nil { | ||
| jobLogGlobal.Warning("[%s] Screenshot capture failed: %v", job.Id, err) | ||
| } | ||
| resultData := struct { | ||
| Screenshot string `json:"screenshot"` | ||
| URL string `json:"url"` | ||
| }{ | ||
| Screenshot: base64Image, | ||
| URL: formatURL(url), | ||
| } | ||
|
|
||
| if jsonBytes, err := json.Marshal(resultData); err != nil { | ||
| jobLogGlobal.ErrorE(fmt.Sprintf("[%s] JSON marshal failed", job.Id), err) | ||
| payload = oasm.NewErrorResult(fmt.Sprintf("JSON error: %v", err)) | ||
| payload = oasm.NewErrorResult(fmt.Sprintf("Screenshot failed: %v", err)) | ||
| } else { | ||
| jsonStr := string(jsonBytes) | ||
| payload = &jobs_registry.DataPayloadResult{ | ||
| Error: false, | ||
| Raw: &jsonStr, | ||
| resultData := struct { | ||
| Screenshot string `json:"screenshot"` | ||
| URL string `json:"url"` | ||
| }{ | ||
| Screenshot: base64Image, | ||
| URL: url, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Return the URL that was actually captured.
TakeScreenshotBase64 normalizes via formatURL, but the result payload stores the raw command input. Normalize once and reuse that value for both capture and resultData.URL.
Proposed fix
url := strings.TrimSpace(after)
+ capturedURL := formatURL(url)
jobLogGlobal.Debug("[%s] Capturing screenshot: %s", job.Id, url)
jobBrowser := NewBrowserSession("screenshot-"+job.Id, toolPath)
- base64Image, err := TakeScreenshotBase64(ctx, jobBrowser, url)
+ base64Image, err := TakeScreenshotBase64(ctx, jobBrowser, capturedURL)
_ = jobBrowser.Close()
@@
}{
Screenshot: base64Image,
- URL: url,
+ URL: capturedURL,
}📝 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.
Suggested change
| jobBrowser := NewBrowserSession("screenshot-"+job.Id, toolPath) | |
| base64Image, err := TakeScreenshotBase64(ctx, jobBrowser, url) | |
| _ = jobBrowser.Close() | |
| if err != nil { | |
| jobLogGlobal.Warning("[%s] Screenshot capture failed: %v", job.Id, err) | |
| } | |
| resultData := struct { | |
| Screenshot string `json:"screenshot"` | |
| URL string `json:"url"` | |
| }{ | |
| Screenshot: base64Image, | |
| URL: formatURL(url), | |
| } | |
| if jsonBytes, err := json.Marshal(resultData); err != nil { | |
| jobLogGlobal.ErrorE(fmt.Sprintf("[%s] JSON marshal failed", job.Id), err) | |
| payload = oasm.NewErrorResult(fmt.Sprintf("JSON error: %v", err)) | |
| payload = oasm.NewErrorResult(fmt.Sprintf("Screenshot failed: %v", err)) | |
| } else { | |
| jsonStr := string(jsonBytes) | |
| payload = &jobs_registry.DataPayloadResult{ | |
| Error: false, | |
| Raw: &jsonStr, | |
| resultData := struct { | |
| Screenshot string `json:"screenshot"` | |
| URL string `json:"url"` | |
| }{ | |
| Screenshot: base64Image, | |
| URL: url, | |
| } | |
| url := strings.TrimSpace(after) | |
| capturedURL := formatURL(url) | |
| jobLogGlobal.Debug("[%s] Capturing screenshot: %s", job.Id, url) | |
| jobBrowser := NewBrowserSession("screenshot-"+job.Id, toolPath) | |
| base64Image, err := TakeScreenshotBase64(ctx, jobBrowser, capturedURL) | |
| _ = jobBrowser.Close() | |
| if err != nil { | |
| jobLogGlobal.Warning("[%s] Screenshot capture failed: %v", job.Id, err) | |
| payload = oasm.NewErrorResult(fmt.Sprintf("Screenshot failed: %v", err)) | |
| } else { | |
| resultData := struct { | |
| Screenshot string `json:"screenshot"` | |
| URL string `json:"url"` | |
| }{ | |
| Screenshot: base64Image, | |
| URL: capturedURL, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@worker/internal/worker/job.go` around lines 51 - 64, The screenshot result is
storing the raw input URL instead of the normalized URL used for capture. In the
job handling flow in job.go, normalize the URL once with the same logic used by
TakeScreenshotBase64/formatURL, then pass that normalized value into both the
screenshot call and resultData.URL so the payload reflects the actual captured
URL.
Comment on lines
+51
to
+60
| base64Image, err := browser.TakeScreenshot(ctx, url, opts) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "timeout") { | ||
| return "", fmt.Errorf("timeout loading page %s", url) | ||
| screenshotLog.Error("timeout loading page %s", url) | ||
| } | ||
| return "", fmt.Errorf("failed to load page %s: %w", url, err) | ||
| } | ||
|
|
||
| quality := 80 | ||
| imgBytes, err := page.Timeout(10*time.Second).Screenshot(true, &proto.PageCaptureScreenshot{ | ||
| Format: proto.PageCaptureScreenshotFormatJpeg, | ||
| Quality: &quality, | ||
| Clip: &proto.PageViewport{ | ||
| X: 0, Y: 0, Width: 1920, Height: 1080, Scale: 1, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to take screenshot: %w", err) | ||
| screenshotLog.Error("failed to load page %s", url) | ||
| } | ||
|
|
||
| screenshotLog.Debug("Screenshot captured successfully: %s", url) | ||
| return base64.StdEncoding.EncodeToString(imgBytes), nil | ||
| return base64Image, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Propagate screenshot capture failures.
browser.TakeScreenshot errors are logged but then discarded, so job.go never sends the intended error payload and may report success with an empty screenshot.
Proposed fix
base64Image, err := browser.TakeScreenshot(ctx, url, opts)
if err != nil {
if strings.Contains(err.Error(), "timeout") {
screenshotLog.Error("timeout loading page %s", url)
}
screenshotLog.Error("failed to load page %s", url)
+ return "", err
}
screenshotLog.Debug("Screenshot captured successfully: %s", url)
return base64Image, nil📝 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.
Suggested change
| base64Image, err := browser.TakeScreenshot(ctx, url, opts) | |
| if err != nil { | |
| if strings.Contains(err.Error(), "timeout") { | |
| return "", fmt.Errorf("timeout loading page %s", url) | |
| screenshotLog.Error("timeout loading page %s", url) | |
| } | |
| return "", fmt.Errorf("failed to load page %s: %w", url, err) | |
| } | |
| quality := 80 | |
| imgBytes, err := page.Timeout(10*time.Second).Screenshot(true, &proto.PageCaptureScreenshot{ | |
| Format: proto.PageCaptureScreenshotFormatJpeg, | |
| Quality: &quality, | |
| Clip: &proto.PageViewport{ | |
| X: 0, Y: 0, Width: 1920, Height: 1080, Scale: 1, | |
| }, | |
| }) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to take screenshot: %w", err) | |
| screenshotLog.Error("failed to load page %s", url) | |
| } | |
| screenshotLog.Debug("Screenshot captured successfully: %s", url) | |
| return base64.StdEncoding.EncodeToString(imgBytes), nil | |
| return base64Image, nil | |
| base64Image, err := browser.TakeScreenshot(ctx, url, opts) | |
| if err != nil { | |
| if strings.Contains(err.Error(), "timeout") { | |
| screenshotLog.Error("timeout loading page %s", url) | |
| } | |
| screenshotLog.Error("failed to load page %s", url) | |
| return "", err | |
| } | |
| screenshotLog.Debug("Screenshot captured successfully: %s", url) | |
| return base64Image, nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@worker/internal/worker/screenshot.go` around lines 51 - 60, The screenshot
capture path in TakeScreenshot is swallowing failures from
browser.TakeScreenshot, which lets the caller continue as if the capture
succeeded. Update TakeScreenshot to return the error immediately after logging
it, and make sure the timeout-specific and general log messages stay in the
error path; this will allow job.go to receive the failure and send the intended
error payload instead of an empty screenshot.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes