Skip to content

Conversation

@Sjoeborg
Copy link
Contributor

@Sjoeborg Sjoeborg commented Sep 16, 2025

Summary

Please include a summary of the change and/or which issues are fixed.

Fixes #1485

List of files changed and why

Dockerfile - Install xvfb for XServer
supervisord.conf - Run xfvb

How Has This Been Tested?

I've tested this by adding
environment:
- DISPLAY=:99
volumes:
- ./supervisord-crawler.conf:/app/supervisord.conf
to my compose file and running crwl https://example.com -b "headless=false" in the container

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added/updated unit tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Summary by CodeRabbit

  • New Features

    • Adds built-in headless display support in the container, enabling graphics-dependent features (e.g., screenshots, visual rendering) to run reliably without a physical display.
  • Chores

    • Container now starts a virtual display service automatically on boot for improved stability of UI-driven tasks.
    • Environment configured to expose the display to applications; logs are routed to standard output/error for easier monitoring.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 16, 2025

Walkthrough

Adds an X virtual framebuffer to the Docker image and supervisord configuration. The Dockerfile sets DISPLAY=:99 and installs xvfb-related packages. Supervisord is updated to launch Xvfb on :99 with defined screen settings and logging, prior to other programs.

Changes

Cohort / File(s) Summary of changes
Docker image setup
Dockerfile
Adds DISPLAY=:99 to ENV. Installs xvfb, xauth, and x11-utils via apt.
Supervisord process config
deploy/docker/supervisord.conf
Adds [program:xvfb] running Xvfb :99 -screen 0 1920x1080x24 -ac +extension GLX +render -noreset as appuser, autorestart=true, priority=5, logs to stdout/stderr with no rotation.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant U as User/Client
    participant S as Supervisord
    participant X as Xvfb (:99)
    participant A as App (Gunicorn)
    participant B as Playwright/Chromium

    rect rgba(230,245,255,0.6)
    note over S: Startup
    S->>X: start Xvfb (:99)
    S->>A: start application
    end

    rect rgba(235,255,235,0.6)
    note over A,B: Runtime
    A->>B: launch browser (DISPLAY=:99)
    B-->>X: connect to X server
    B-->>A: browser ready
    U->>A: request with headless=false
    A->>B: open page
    B-->>X: render to virtual display
    B-->>A: results/events
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my nose at dawn’s first light,
Now Xs bloom in virtual night—
On display ninety-nine I hop,
Screens appear without a stop.
With xvfb my whiskers twirl,
Headed dreams in Docker whirl.
Thump-thump: ship it, bunny-girl! 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly and accurately summarizes the primary change: adding Xvfb to the Docker image to enable headed Playwright crawls, and it directly reflects the modified files (Dockerfile and supervisord.conf) and the linked issue (#1485).
Linked Issues Check ✅ Passed The changes implement the requested fix from issue #1485 by installing Xvfb in the Docker image, adding DISPLAY=:99 to the environment, and adding a supervisord [program:xvfb] that starts Xvfb with priority 5 so it will run before gunicorn; these edits address the "no X server" cause of Playwright's headed-mode failure.
Out of Scope Changes Check ✅ Passed All modified files listed in the summary (Dockerfile and deploy/docker/supervisord.conf) are directly related to providing an X server for headed Playwright runs; there are no unrelated or extraneous code changes in the provided summary.
Description Check ✅ Passed The PR description follows the repository template by providing a Summary (including "Fixes #1485"), a list of files changed with reasons, a description of how the change was tested, and a checklist; the remaining gaps are unchecked checklist items for documentation and automated tests.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Dockerfile (1)

47-47: Broken line continuations: backslash followed by a space will break the build.

Each of these RUN chains has “apt-get clean \ ” (space after “\”), so “&& rm …” starts a new shell statement and fails with “syntax error near unexpected token '&&'”.

Apply this diff to remove the trailing spaces after the backslashes:

-    && apt-get clean \ 
+    && apt-get clean \
...
-    && apt-get clean \ 
+    && apt-get clean \
...
-    && apt-get clean \ 
+    && apt-get clean \
...
-    && apt-get clean \ 
+    && apt-get clean \
...
-    && apt-get clean \ 
+    && apt-get clean \

Also applies to: 75-75, 84-84, 94-94, 100-100

🧹 Nitpick comments (4)
Dockerfile (4)

72-75: Right deps for Xvfb; consider readiness and minimalism.

  • Packages look correct for a virtual X server; x11-utils gives xdpyinfo we can use for readiness checks.
  • If you won’t use xauth, drop it; otherwise, prefer auth over “-ac” in supervisord (see comment there).

Would you like me to wire a simple readiness check using xdpyinfo in the HEALTHCHECK?


186-194: Add X server readiness to HEALTHCHECK (cheap, prevents flakiness).

Since x11-utils is installed, verify Xvfb on :99 before probing the app.

 HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 \
     CMD bash -c '\
     MEM=$(free -m | awk "/^Mem:/{print \$2}"); \
     if [ $MEM -lt 2048 ]; then \
         echo "⚠️ Warning: Less than 2GB RAM available! Your container might need a memory boost! 🚀"; \
         exit 1; \
     fi && \
+    xdpyinfo -display :99 >/dev/null 2>&1 && \
     redis-cli ping > /dev/null && \
     curl -f http://localhost:11235/health || exit 1'

130-130: Duplicate copy of supervisord.conf.

You copy supervisord.conf here and again later via “COPY deploy/docker/* …”. Keep one to avoid drift.

-COPY deploy/docker/supervisord.conf .

200-201: Typo: “ptoduction”.

Set PYTHON_ENV to “production”.

-# Set environment variables to ptoduction
+# Set environment variables to production
 ENV PYTHON_ENV=production 
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e651e04 and dc8a49b.

📒 Files selected for processing (2)
  • Dockerfile (2 hunks)
  • deploy/docker/supervisord.conf (1 hunks)
🔇 Additional comments (1)
Dockerfile (1)

23-24: Good: propagate DISPLAY for headful runs.

Setting DISPLAY=:99 matches the Xvfb server choice. No issues.

Please confirm no other process in the image sets/overrides DISPLAY at runtime (e.g., via shell profile or entrypoint).

Comment on lines +28 to +36
[program:xvfb]
command=Xvfb :99 -screen 0 1920x1080x24 -ac +extension GLX +render -noreset
user=appuser
autorestart=true
priority=5
stdout_logfile=/dev/stdout
stdout_logfile_maxbytes=0
stderr_logfile=/dev/stderr
stderr_logfile_maxbytes=0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Secure and stabilize Xvfb startup.

  • Security: Xvfb is started with “-ac” (no auth). Add “-nolisten tcp” to disable TCP socket exposure. If feasible, drop “-ac” and use xauth, since you already install it.
  • Reliability: Add startsecs so Xvfb is “up” before gunicorn starts.
 [program:xvfb]
-command=Xvfb :99 -screen 0 1920x1080x24 -ac +extension GLX +render -noreset
+command=Xvfb :99 -screen 0 1920x1080x24 -nolisten tcp -ac +extension GLX +render -noreset
 user=appuser
 autorestart=true
 priority=5
+startsecs=2
 stdout_logfile=/dev/stdout
 stdout_logfile_maxbytes=0
 stderr_logfile=/dev/stderr
 stderr_logfile_maxbytes=0

Optional hardening (if you choose to use xauth instead of “-ac”): generate a cookie at container start and export XAUTHORITY to both programs.

I can provide a tiny entry script + supervisor config to set up xauth securely.

📝 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
[program:xvfb]
command=Xvfb :99 -screen 0 1920x1080x24 -ac +extension GLX +render -noreset
user=appuser
autorestart=true
priority=5
stdout_logfile=/dev/stdout
stdout_logfile_maxbytes=0
stderr_logfile=/dev/stderr
stderr_logfile_maxbytes=0
[program:xvfb]
command=Xvfb :99 -screen 0 1920x1080x24 -nolisten tcp -ac +extension GLX +render -noreset
user=appuser
autorestart=true
priority=5
startsecs=2
stdout_logfile=/dev/stdout
stdout_logfile_maxbytes=0
stderr_logfile=/dev/stderr
stderr_logfile_maxbytes=0
🤖 Prompt for AI Agents
In deploy/docker/supervisord.conf around lines 28 to 36 the Xvfb command is
insecure and may race with gunicorn start; remove or replace the “-ac” flag
(prefer using xauth) and add “-nolisten tcp” to prevent TCP exposure, update the
supervisord program stanza to include a sensible startsecs (e.g. startsecs=5) so
supervisor waits for Xvfb to be fully up before starting dependent programs, and
if you opt to drop “-ac” implement an entrypoint step that generates an
XAUTHORITY cookie (export XAUTHORITY for both Xvfb and other processes) rather
than disabling auth.

@ntohidi ntohidi changed the base branch from main to develop September 18, 2025 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Docker server does not support headed crawling

1 participant