Skip to content

Fix screenshot downloader #2326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

CoolSoulz
Copy link

Description

Issue Fixes

None

Checklist:

  • I am pushing changes to the develop branch
  • I am using the recommended development environment
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have formatted and linted my code using python-black and pylint
  • I have cleaned up unnecessary files
  • My changes generate no new warnings
  • My changes follow the existing code-style
  • My changes are relevant to the project

Any other information (e.g how to test the changes)

None

@Cyteon Cyteon requested a review from Copilot June 5, 2025 09:15
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the screenshot downloader to use custom Shreddit components, adds logic to ensure comment content is visible before capturing, and adjusts zoom/clip calculations.

  • Swapped raw CSS selectors ([data-test-id], #t1_…) for shreddit-post and shreddit-comment tags
  • Introduced retry loop to reveal hidden comment elements before screenshotting
  • Updated scroll, zoom, and bounding box logic to use the new target selector
Comments suppressed due to low confidence (2)

video_creation/screenshot_downloader.py:180

  • [nitpick] The variable name location is a dictionary of clipping coordinates; consider renaming it to something like bbox or clip_box for clarity.
location = page.locator('shreddit-post').bounding_box()

video_creation/screenshot_downloader.py:247

  • [nitpick] The inline comment includes Chinese (或 'inline'), which may be unclear to some developers; consider translating or clarifying the intended alternative behavior.
element.style.display = 'block'; // 或 'inline'

visible = page.locator(target).is_visible()

if not visible:
class ElementVisible(Exception):pass
Copy link
Preview

Copilot AI Jun 5, 2025

Choose a reason for hiding this comment

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

Defining a custom exception inside the try block for control flow can be confusing; consider using a simple flag or breaking out of the loop instead.

Copilot uses AI. Check for mistakes.

visible = page.locator(target).is_visible()
if visible:
raise ElementVisible
target = f'shreddit-comment[thingid="t1_{comment["comment_id"]}"] div#t1_{comment["comment_id"]}-comment-rtjson-content div#-post-rtjson-content'
Copy link
Preview

Copilot AI Jun 5, 2025

Choose a reason for hiding this comment

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

The selector div#-post-rtjson-content looks like it’s missing the dynamic prefix; it should include the comment ID (e.g., div#t1_{comment_id}-post-rtjson-content).

Suggested change
target = f'shreddit-comment[thingid="t1_{comment["comment_id"]}"] div#t1_{comment["comment_id"]}-comment-rtjson-content div#-post-rtjson-content'
target = f'shreddit-comment[thingid="t1_{comment["comment_id"]}"] div#t1_{comment["comment_id"]}-comment-rtjson-content div#t1_{comment["comment_id"]}-post-rtjson-content'

Copilot uses AI. Check for mistakes.

if not visible:
class ElementVisible(Exception):pass
try:
for _ in range(30):
Copy link
Preview

Copilot AI Jun 5, 2025

Choose a reason for hiding this comment

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

Polling every second for up to 30 seconds may slow down overall execution; consider using Playwright’s page.wait_for_selector(target, timeout=30000) instead.

Suggested change
for _ in range(30):
page.evaluate("""
(target) => {
const element = document.querySelector(target);
if (element) {
element.style.display = 'block'; // 'inline'
element.style.visibility = 'visible';
}
}
""", target)
page.wait_for_selector(target, timeout=30000)
target = f'shreddit-comment[thingid="t1_{comment["comment_id"]}"] div#t1_{comment["comment_id"]}-comment-rtjson-content div#-post-rtjson-content'
visible = page.locator(target).is_visible()
if not visible:

Copilot uses AI. Check for mistakes.

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.

1 participant