Make tests more resilient#124
Merged
Merged
Conversation
Signed-off-by: Luca Foppiano <luca@foppiano.org>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces non-asserting Playwright visibility checks with Changes
Sequence Diagram(s)(omitted — conditions for diagram generation not met) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 (6)
tests/test_edge_cases.py (1)
147-147: Replace fixed sleep with a deterministic wait after viewport resize.Line 147 uses a hard delay after
page.set_viewport_size(), which is a common flake source in Playwright tests. Instead of waiting a fixed duration, wait for the viewport dimensions to actually be applied:♻️ Proposed change
for viewport in viewports: page.set_viewport_size(viewport) - page.wait_for_timeout(500) + page.wait_for_function( + "(size) => window.innerWidth === size.width && window.innerHeight === size.height", + viewport, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_edge_cases.py` at line 147, Replace the fixed sleep after page.set_viewport_size with a deterministic wait that verifies the viewport was applied: after calling page.set_viewport_size(...) (the symbol to change), call a wait that polls the page (e.g., page.wait_for_function or an equivalent helper) to assert window.innerWidth and window.innerHeight match the expected dimensions instead of page.wait_for_timeout(500); this ensures the test proceeds only when the viewport change actually took effect.tests/test_resolution_boost.py (1)
85-85: Replace fixed timeout with explicit state-based wait for consistency and reliability.This is the only fixed sleep in the test's critical path. The rest of the file uses Playwright's
.wait_for(state="visible")pattern (lines 39–40, 59–113). Replacepage.wait_for_timeout(500)with an explicit wait for the elements thatwait_for_canvases()depends on, such as waiting for the iframe or its child elements to become visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_resolution_boost.py` at line 85, Replace the fixed sleep page.wait_for_timeout(500) with an explicit state-based wait for the elements wait_for_canvases() depends on: wait for the iframe and/or its child canvas elements to be visible using Playwright's wait_for_selector or wait_for with state="visible" before calling wait_for_canvases(); update the test to target the same iframe/canvas selectors already used elsewhere in the file so the test waits deterministically for the UI to be ready rather than using a hard timeout.tests/test_accessibility.py (1)
110-110: Replace fixed timeout with condition-based wait after viewport resize.
page.wait_for_timeout(500)is timing-sensitive and can cause flakiness in CI. The same test suite already uses condition-based waits elsewhere (e.g.,locator.wait_for(state='visible')). Replace this with a wait on the iframe or pdfViewer to be visible, following the pattern used in test_resolution_boost.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_accessibility.py` at line 110, Replace the brittle fixed sleep in tests/test_accessibility.py (the page.wait_for_timeout(500) call) with a condition-based wait: locate the iframe or the pdfViewer element used in this test and call its locator.wait_for(state='visible') (or equivalent visible/state check) to wait for the resize to complete, following the pattern used in test_resolution_boost.py; update the test to wait on the same unique selector used elsewhere (iframe or pdfViewer locator) rather than using page.wait_for_timeout.tests/test_rendering_text.py (2)
103-114: Consider usingwait_for_canvasesfor render stability.The removed delays (3000ms + 2000ms per AI summary) were likely compensating for PDF rendering time. The current approach only waits for the first canvas to be visible in the DOM (
canvas.wait_for(state='visible')), which confirms DOM presence but not render completion.Per the relevant code snippets, other tests use
wait_for_canvases()which polls until canvas count stabilizes, providing better render-completion guarantees. Since this test verifies text content visibility (line 116-118), incomplete rendering could cause flakiness.💡 Suggested improvement
+from tests import wait_for_canvases + # Wait for canvas elements to be rendered (PDF content) - canvas = pdf_viewer_1.locator('canvas').first - canvas.wait_for(timeout=10000, state='visible') + canvas_locator = pdf_viewer_1.locator('canvas') + wait_for_canvases(canvas_locator)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_rendering_text.py` around lines 103 - 114, Replace the single DOM presence check on the first canvas with the shared render-stability helper: call wait_for_canvases(pdf_viewer_1) (the test-suite helper that polls until canvas count stabilizes) before measuring pdf_container_1.bounding_box() and before the text-visibility assertions; remove the canvas.wait_for(...) usage and ensure wait_for_canvases is imported/available in this test so the PDF rendering is fully completed prior to the size and text assertions.
112-114: Height assertion range is overly permissive.The range
299 <= height <= 8000provides minimal validation. If the expected height is ~300px (based on similar tests liketest_tabs_setHeight.pyline 90), consider tightening this:- assert 299 <= b_box_1['height'] <= 8000 + assert 299 <= b_box_1['height'] <= 301If the height genuinely varies widely, document why in a comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_rendering_text.py` around lines 112 - 114, The height assertion on b_box_1 from bounding_box() is too loose (299 <= height <= 8000); tighten it to validate expected ~300px by asserting a narrower range (e.g., 290 <= b_box_1['height'] <= 350) or use a proximity check (abs(b_box_1['height'] - 300) <= 10 or pytest.approx(300, rel=0.05)); if the height truly can vary widely, add a short comment above the assertion explaining why the larger range is necessary and reference the similar expectation in test_tabs_setHeight.py and the bounding_box()/b_box_1 symbols to locate the check.tests/test_unwrap_no_args.py (1)
97-104: Reduced wait time may introduce flakiness for viewport responsiveness test.The wait after
set_viewport_sizewas reduced from 1000ms to 500ms. Since the assertion at line 104 checks that dimensions have changed, if the resize or re-layout takes longer than 500ms, this test may become flaky.Consider using a polling approach or an element-based wait (e.g., waiting for the bounding box to change) rather than a fixed timeout for more resilient synchronization.
💡 Alternative: poll until dimensions change
# Example approach using expect with a custom condition from playwright.sync_api import expect # After set_viewport_size, poll until the box changes def wait_for_resize(): for _ in range(20): # 20 * 100ms = 2s max new_box = iframe_component.bounding_box() if new_box['width'] != initial_width or new_box['height'] != initial_height: return new_box page.wait_for_timeout(100) return iframe_component.bounding_box() new_frame_box = wait_for_resize()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_unwrap_no_args.py` around lines 97 - 104, The 500ms fixed sleep after page.set_viewport_size is flaky; replace the static wait with a polling or element-based wait that repeatedly calls iframe_component.bounding_box() until its width or height differs from initial_width/initial_height (or a timeout is reached), removing page.wait_for_timeout(500) and using a loop/expect-like retry to return the new_frame_box only when the dimensions change (or fail after a sane max wait).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_interactive_features.py`:
- Around line 18-20: The test locates zoom_controls via
iframe_frame.locator('button.zoom-button, .zoom-controls, [class*="zoom"]') but
contains no assertions; update the test to either assert presence (e.g., expect
the locator to have a positive count and to be visible) and/or verify basic
functionality (click the control via zoom_controls.first() and assert the
expected change in the UI or the iframe content), or remove the locator and the
test if zoom functionality cannot be reliably tested; reference the
zoom_controls locator and iframe_frame.locator when making the changes.
---
Nitpick comments:
In `@tests/test_accessibility.py`:
- Line 110: Replace the brittle fixed sleep in tests/test_accessibility.py (the
page.wait_for_timeout(500) call) with a condition-based wait: locate the iframe
or the pdfViewer element used in this test and call its
locator.wait_for(state='visible') (or equivalent visible/state check) to wait
for the resize to complete, following the pattern used in
test_resolution_boost.py; update the test to wait on the same unique selector
used elsewhere (iframe or pdfViewer locator) rather than using
page.wait_for_timeout.
In `@tests/test_edge_cases.py`:
- Line 147: Replace the fixed sleep after page.set_viewport_size with a
deterministic wait that verifies the viewport was applied: after calling
page.set_viewport_size(...) (the symbol to change), call a wait that polls the
page (e.g., page.wait_for_function or an equivalent helper) to assert
window.innerWidth and window.innerHeight match the expected dimensions instead
of page.wait_for_timeout(500); this ensures the test proceeds only when the
viewport change actually took effect.
In `@tests/test_rendering_text.py`:
- Around line 103-114: Replace the single DOM presence check on the first canvas
with the shared render-stability helper: call wait_for_canvases(pdf_viewer_1)
(the test-suite helper that polls until canvas count stabilizes) before
measuring pdf_container_1.bounding_box() and before the text-visibility
assertions; remove the canvas.wait_for(...) usage and ensure wait_for_canvases
is imported/available in this test so the PDF rendering is fully completed prior
to the size and text assertions.
- Around line 112-114: The height assertion on b_box_1 from bounding_box() is
too loose (299 <= height <= 8000); tighten it to validate expected ~300px by
asserting a narrower range (e.g., 290 <= b_box_1['height'] <= 350) or use a
proximity check (abs(b_box_1['height'] - 300) <= 10 or pytest.approx(300,
rel=0.05)); if the height truly can vary widely, add a short comment above the
assertion explaining why the larger range is necessary and reference the similar
expectation in test_tabs_setHeight.py and the bounding_box()/b_box_1 symbols to
locate the check.
In `@tests/test_resolution_boost.py`:
- Line 85: Replace the fixed sleep page.wait_for_timeout(500) with an explicit
state-based wait for the elements wait_for_canvases() depends on: wait for the
iframe and/or its child canvas elements to be visible using Playwright's
wait_for_selector or wait_for with state="visible" before calling
wait_for_canvases(); update the test to target the same iframe/canvas selectors
already used elsewhere in the file so the test waits deterministically for the
UI to be ready rather than using a hard timeout.
In `@tests/test_unwrap_no_args.py`:
- Around line 97-104: The 500ms fixed sleep after page.set_viewport_size is
flaky; replace the static wait with a polling or element-based wait that
repeatedly calls iframe_component.bounding_box() until its width or height
differs from initial_width/initial_height (or a timeout is reached), removing
page.wait_for_timeout(500) and using a loop/expect-like retry to return the
new_frame_box only when the dimensions change (or fail after a sane max wait).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c57342bc-70b8-483d-a905-38b1d4c9b5b1
📒 Files selected for processing (27)
tests/conftest.pytests/e2e_utils.pytests/test_accessibility.pytests/test_alignment_center.pytests/test_alignment_left.pytests/test_alignment_right.pytests/test_browser_compatibility.pytests/test_edge_cases.pytests/test_error_handling.pytests/test_interactive_features.pytests/test_invalid_params.pytests/test_multiple_instances.pytests/test_page_separator_disabled.pytests/test_page_separator_enabled.pytests/test_performance.pytests/test_rendering_text.pytests/test_resolution_boost.pytests/test_responsive_design.pytests/test_tabs_setHeight.pytests/test_tabs_setWidth.pytests/test_unwrap_height.pytests/test_unwrap_no_args.pytests/test_unwrap_width.pytests/test_unwrap_width_height.pytests/test_zoom_auto.pytests/test_zoom_auto_height.pytests/test_zoom_numeric.py
💤 Files with no reviewable changes (4)
- tests/test_browser_compatibility.py
- tests/test_error_handling.py
- tests/test_multiple_instances.py
- tests/test_performance.py
Signed-off-by: Luca Foppiano <luca@foppiano.org>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
streamlit_pdf_viewer/frontend/src/PdfViewer.vue (1)
490-490: Cancel pending debounced resize on unmount.Pending debounced calls can fire after component unmount. Add
debouncedHandleResize.cancel()to clear the timer queue during cleanup.🔧 Proposed fix
onUnmounted(() => { window.removeEventListener("resize", debouncedHandleResize); document.removeEventListener('click', handleClickOutside); + debouncedHandleResize.cancel(); observer.disconnect(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@streamlit_pdf_viewer/frontend/src/PdfViewer.vue` at line 490, The component cleanup currently only calls observer.disconnect() but doesn't cancel the pending debounced resize, which can run after unmount; update the unmount/cleanup routine to call debouncedHandleResize.cancel() in addition to observer.disconnect() so any queued debouncedHandleResize timers are cleared (locate the cleanup that currently calls observer.disconnect() and add a call to debouncedHandleResize.cancel()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@streamlit_pdf_viewer/frontend/src/PdfViewer.vue`:
- Around line 468-476: The IntersectionObserver is created unconditionally which
can crash in environments that don't support it; before instantiating the const
observer = new IntersectionObserver(...), add a feature check for
window?.IntersectionObserver (or typeof IntersectionObserver !== 'undefined')
and only create and use observer when available—otherwise ensure
wasVisible/value and any teardown logic (e.g., observer.disconnect) gracefully
skip usage; keep references to wasVisible.value and debouncedHandleResize inside
the guarded block so their calls only occur when IntersectionObserver exists.
---
Nitpick comments:
In `@streamlit_pdf_viewer/frontend/src/PdfViewer.vue`:
- Line 490: The component cleanup currently only calls observer.disconnect() but
doesn't cancel the pending debounced resize, which can run after unmount; update
the unmount/cleanup routine to call debouncedHandleResize.cancel() in addition
to observer.disconnect() so any queued debouncedHandleResize timers are cleared
(locate the cleanup that currently calls observer.disconnect() and add a call to
debouncedHandleResize.cancel()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c2c5d65-0c5c-4516-9e97-0fe7e8cc9349
📒 Files selected for processing (1)
streamlit_pdf_viewer/frontend/src/PdfViewer.vue
Comment on lines
+468
to
+476
| const observer = new IntersectionObserver((entries) => { | ||
| const entry = entries[0]; | ||
| if (entry.isIntersecting && !wasVisible.value) { | ||
| wasVisible.value = true; | ||
| debouncedHandleResize(); | ||
| } else if (!entry.isIntersecting) { | ||
| wasVisible.value = false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether IntersectionObserver is already polyfilled/mocked in this repo.
rg -n -C2 'IntersectionObserver|mock.*IntersectionObserver|polyfill' -g '!**/node_modules/**'Repository: lfoppiano/streamlit-pdf-viewer
Length of output: 545
🏁 Script executed:
#!/bin/bash
# Check package.json for dependencies and browser targets
head -50 package.json
# Check for browserslist or similar browser target config
fd -e '.browserslistrc|browserslist|package.json' . --max-depth 2
# Search for test setup that might use JSDOM or similar
rg -l 'jest|vitest|mocha|testEnvironment' --max-count 5Repository: lfoppiano/streamlit-pdf-viewer
Length of output: 145
🏁 Script executed:
#!/bin/bash
# Find package.json in the frontend directory
find . -name "package.json" -type f | head -5
# Look for test configuration files
find . -name "vitest.config.*" -o -name "jest.config.*" -o -name "vite.config.*" | head -10
# Check if tests exist and what environment they use
rg -l 'describe|it\(|test\(' --type javascript --type typescript -g '**/*.test.*|**/*.spec.*' | head -5Repository: lfoppiano/streamlit-pdf-viewer
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Read the frontend package.json
cat ./streamlit_pdf_viewer/frontend/package.json
# Look for vitest/jest config in frontend
find ./streamlit_pdf_viewer/frontend -maxdepth 2 -name "vitest.config.*" -o -name "jest.config.*" -o -name "vite.config.*"
# Check for test files
find ./streamlit_pdf_viewer/frontend -name "*.test.*" -o -name "*.spec.*" | head -5Repository: lfoppiano/streamlit-pdf-viewer
Length of output: 865
🏁 Script executed:
#!/bin/bash
# Check the main entry point for core-js imports
find ./streamlit_pdf_viewer/frontend/src -name "main.*" -o -name "index.*" | head -5
# Search for core-js imports
rg 'core-js|import.*polyfill' ./streamlit_pdf_viewer/frontend/src
# Check if IntersectionObserver is used elsewhere in the codebase with guards
rg -B3 -A3 'IntersectionObserver' ./streamlit_pdf_viewer/frontend/src
# Look for any browser support documentation
find ./streamlit_pdf_viewer -maxdepth 3 -name "README*" -o -name "BROWSER*" | xargs cat 2>/dev/null | head -100Repository: lfoppiano/streamlit-pdf-viewer
Length of output: 838
🏁 Script executed:
#!/bin/bash
# Read main.ts to check for polyfill setup
cat ./streamlit_pdf_viewer/frontend/src/main.tsRepository: lfoppiano/streamlit-pdf-viewer
Length of output: 163
Guard IntersectionObserver initialization to prevent runtime failures in unsupported environments.
Line 468 creates new IntersectionObserver(...) unconditionally. This will crash at component setup time in environments lacking the API (older browsers, server-side rendering, or test runners using JSDOM). Add a feature check before instantiation.
🔧 Proposed fix
- const observer = new IntersectionObserver((entries) => {
- const entry = entries[0];
- if (entry.isIntersecting && !wasVisible.value) {
- wasVisible.value = true;
- debouncedHandleResize();
- } else if (!entry.isIntersecting) {
- wasVisible.value = false;
- }
- });
+ let observer = null;
+ if (typeof window !== "undefined" && "IntersectionObserver" in window) {
+ observer = new IntersectionObserver((entries) => {
+ const entry = entries[0];
+ if (entry.isIntersecting && !wasVisible.value) {
+ wasVisible.value = true;
+ debouncedHandleResize();
+ } else if (!entry.isIntersecting) {
+ wasVisible.value = false;
+ }
+ });
+ }📝 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
| const observer = new IntersectionObserver((entries) => { | |
| const entry = entries[0]; | |
| if (entry.isIntersecting && !wasVisible.value) { | |
| wasVisible.value = true; | |
| debouncedHandleResize(); | |
| } else if (!entry.isIntersecting) { | |
| wasVisible.value = false; | |
| } | |
| }); | |
| let observer = null; | |
| if (typeof window !== "undefined" && "IntersectionObserver" in window) { | |
| observer = new IntersectionObserver((entries) => { | |
| const entry = entries[0]; | |
| if (entry.isIntersecting && !wasVisible.value) { | |
| wasVisible.value = true; | |
| debouncedHandleResize(); | |
| } else if (!entry.isIntersecting) { | |
| wasVisible.value = false; | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@streamlit_pdf_viewer/frontend/src/PdfViewer.vue` around lines 468 - 476, The
IntersectionObserver is created unconditionally which can crash in environments
that don't support it; before instantiating the const observer = new
IntersectionObserver(...), add a feature check for window?.IntersectionObserver
(or typeof IntersectionObserver !== 'undefined') and only create and use
observer when available—otherwise ensure wasVisible/value and any teardown logic
(e.g., observer.disconnect) gracefully skip usage; keep references to
wasVisible.value and debouncedHandleResize inside the guarded block so their
calls only occur when IntersectionObserver exists.
Signed-off-by: Luca Foppiano <luca@foppiano.org>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
streamlit_pdf_viewer/frontend/src/PdfViewer.vue (1)
470-477:⚠️ Potential issue | 🟠 MajorGuard
ResizeObserverconstruction for unsupported environments.
new ResizeObserver(...)is created unconditionally; this can fail in environments where the API is unavailable (older browsers/test runners). Please gate creation and lifecycle calls.🔧 Proposed fix
- let lastContainerWidth = 0; - const resizeObserver = new ResizeObserver((entries) => { - const entry = entries[0]; - const newWidth = entry.contentRect.width; - if (lastContainerWidth === 0 && newWidth > 0) { - debouncedHandleResize(); - } - lastContainerWidth = newWidth; - }); + let lastContainerWidth = 0; + const resizeObserver = + typeof window !== "undefined" && "ResizeObserver" in window + ? new ResizeObserver((entries) => { + const entry = entries[0]; + const newWidth = entry.contentRect.width; + if (lastContainerWidth === 0 && newWidth > 0) { + debouncedHandleResize(); + } + lastContainerWidth = newWidth; + }) + : null; @@ - if (pdfContainer.value) { + if (resizeObserver && pdfContainer.value) { resizeObserver.observe(pdfContainer.value); } @@ - resizeObserver.disconnect(); + resizeObserver?.disconnect();#!/bin/bash # Verify whether ResizeObserver is polyfilled/mocked or already guarded in frontend. rg -n -C2 'ResizeObserver|mock.*ResizeObserver|polyfill' streamlit_pdf_viewer/frontend🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@streamlit_pdf_viewer/frontend/src/PdfViewer.vue` around lines 470 - 477, Wrap creation and use of ResizeObserver in a feature-check so it only runs when the API exists: guard the new ResizeObserver(...) instantiation and any calls on the resizeObserver variable (observe, disconnect, etc.) with a conditional like "if (typeof ResizeObserver !== 'undefined')" around the block where resizeObserver is defined and where it is used; ensure debouncedHandleResize() and lastContainerWidth logic remain unchanged inside the guarded block and that the resizeObserver variable is null/undefined-safe for lifecycle cleanup to avoid runtime errors in environments without ResizeObserver.
🧹 Nitpick comments (1)
streamlit_pdf_viewer/frontend/src/PdfViewer.vue (1)
473-476: Observer condition is too narrow for non-window layout changes.Current logic only reacts to
0 -> >0. Consider also reacting to>0 -> different >0so width changes from parent layout updates can reflow pages reliably.♻️ Suggested adjustment
- if (lastContainerWidth === 0 && newWidth > 0) { + if (newWidth > 0 && newWidth !== lastContainerWidth) { debouncedHandleResize(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@streamlit_pdf_viewer/frontend/src/PdfViewer.vue` around lines 473 - 476, The current resize observer only triggers debouncedHandleResize when lastContainerWidth transitions from 0 to >0; update the condition in the observer so it also triggers when lastContainerWidth > 0 and newWidth differs (lastContainerWidth !== newWidth) to handle parent layout-driven width changes; keep assigning lastContainerWidth = newWidth after the check so future changes are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@streamlit_pdf_viewer/frontend/src/PdfViewer.vue`:
- Around line 470-477: Wrap creation and use of ResizeObserver in a
feature-check so it only runs when the API exists: guard the new
ResizeObserver(...) instantiation and any calls on the resizeObserver variable
(observe, disconnect, etc.) with a conditional like "if (typeof ResizeObserver
!== 'undefined')" around the block where resizeObserver is defined and where it
is used; ensure debouncedHandleResize() and lastContainerWidth logic remain
unchanged inside the guarded block and that the resizeObserver variable is
null/undefined-safe for lifecycle cleanup to avoid runtime errors in
environments without ResizeObserver.
---
Nitpick comments:
In `@streamlit_pdf_viewer/frontend/src/PdfViewer.vue`:
- Around line 473-476: The current resize observer only triggers
debouncedHandleResize when lastContainerWidth transitions from 0 to >0; update
the condition in the observer so it also triggers when lastContainerWidth > 0
and newWidth differs (lastContainerWidth !== newWidth) to handle parent
layout-driven width changes; keep assigning lastContainerWidth = newWidth after
the check so future changes are detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69a149da-76c9-435d-a2c0-cd408ad03e0d
📒 Files selected for processing (1)
streamlit_pdf_viewer/frontend/src/PdfViewer.vue
Signed-off-by: Luca Foppiano <luca@foppiano.org>
Signed-off-by: Luca Foppiano <luca@foppiano.org>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/ci-build.yml (1)
24-30: Harden action references by pinning immutable SHAs.Lines 24, 26, and 30 use mutable major tags (
@v6). Pin to full commit SHAs to reduce supply-chain risk:
- Line 24:
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd- Line 26:
actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405- Line 30:
actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-build.yml around lines 24 - 30, Replace the mutable action tags with the provided immutable commit SHAs: change the uses: reference for actions/checkout (currently actions/checkout@v6) to actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd, actions/setup-python (currently actions/setup-python@v6) to actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405, and actions/setup-node (currently actions/setup-node@v6) to actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f so the workflow uses pinned immutable SHAs instead of major tags.tests/test_unwrap_no_args.py (1)
92-92: Replace fixed sleep with state-based waiting for iframe dimension changes.Line 92 relies on
wait_for_timeout(500), which can be flaky under variable CI load. The variablesinitial_widthandinitial_heightare already captured at lines 87–88; usewait_for_function()to wait for the iframe's actual dimensions to change instead.🔧 Suggested refactor
# Change viewport size page.set_viewport_size({"width": 600, "height": 400}) - page.wait_for_timeout(500) # Wait for responsive adjustment + page.wait_for_function( + """([prevW, prevH]) => { + const iframe = document.querySelector('iframe[title="streamlit_pdf_viewer.streamlit_pdf_viewer"]'); + if (!iframe) return false; + const r = iframe.getBoundingClientRect(); + return r.width !== prevW || r.height !== prevH; + }""", + [initial_width, initial_height], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_unwrap_no_args.py` at line 92, Replace the hard-coded sleep page.wait_for_timeout(500) with a state-based wait that polls the iframe's dimensions until they differ from the previously captured initial_width and initial_height; use page.wait_for_function() (or frame.evaluate) to query the iframe element's offsetWidth/offsetHeight and return when width !== initial_width || height !== initial_height. Update the call site where initial_width and initial_height are captured (variables initial_width, initial_height) and remove the fixed timeout, ensuring the selector used to find the iframe matches the existing code that set those variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_interactive_features.py`:
- Around line 10-12: Update the misleading inline comment to match the actual
assertion: change the comment referencing “all three PDF viewers” to reflect
that the test expects a single PDF viewer. Locate the test variable
iframe_components and the locator
'iframe[title="streamlit_pdf_viewer.streamlit_pdf_viewer"]' in
tests/test_interactive_features.py and revise the comment above that assertion
so it accurately describes expect(iframe_components).to_have_count(1).
In `@tests/test_unwrap_no_args.py`:
- Line 72: The loop introduces an unused enumerate index `i` causing lint
warning B007; update the for-loop over canvas_locator.all() to replace the
unused `i` with `_` (i.e., use "for _, canvas in
enumerate(canvas_locator.all()):" or simply "for canvas in
canvas_locator.all():" depending on whether the index is needed) so the variable
`i` is removed and the Ruff warning is resolved.
---
Nitpick comments:
In @.github/workflows/ci-build.yml:
- Around line 24-30: Replace the mutable action tags with the provided immutable
commit SHAs: change the uses: reference for actions/checkout (currently
actions/checkout@v6) to
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd, actions/setup-python
(currently actions/setup-python@v6) to
actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405, and
actions/setup-node (currently actions/setup-node@v6) to
actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f so the workflow uses
pinned immutable SHAs instead of major tags.
In `@tests/test_unwrap_no_args.py`:
- Line 92: Replace the hard-coded sleep page.wait_for_timeout(500) with a
state-based wait that polls the iframe's dimensions until they differ from the
previously captured initial_width and initial_height; use
page.wait_for_function() (or frame.evaluate) to query the iframe element's
offsetWidth/offsetHeight and return when width !== initial_width || height !==
initial_height. Update the call site where initial_width and initial_height are
captured (variables initial_width, initial_height) and remove the fixed timeout,
ensuring the selector used to find the iframe matches the existing code that set
those variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7d10004-feea-44df-a1cf-1472d8d69822
📒 Files selected for processing (9)
.github/workflows/ci-build.yml.github/workflows/ci-bump-version.yml.github/workflows/ci-release.ymltests/test_interactive_features.pytests/test_tabs_setHeight.pytests/test_unwrap_height.pytests/test_unwrap_no_args.pytests/test_unwrap_width.pytests/test_unwrap_width_height.py
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/ci-bump-version.yml
- .github/workflows/ci-release.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_unwrap_width_height.py
- tests/test_unwrap_height.py
- tests/test_unwrap_width.py
- tests/test_tabs_setHeight.py
Comment on lines
10
to
12
| # Check that all three PDF viewers are present | ||
| iframe_components = page.locator('iframe[title="streamlit_pdf_viewer.streamlit_pdf_viewer"]') | ||
| expect(iframe_components).to_have_count(1) |
There was a problem hiding this comment.
Update stale inline comment about iframe count.
Line 10 says “all three PDF viewers,” but Line 12 asserts a count of 1. Please align the comment with the actual expectation to avoid confusion during future maintenance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_interactive_features.py` around lines 10 - 12, Update the
misleading inline comment to match the actual assertion: change the comment
referencing “all three PDF viewers” to reflect that the test expects a single
PDF viewer. Locate the test variable iframe_components and the locator
'iframe[title="streamlit_pdf_viewer.streamlit_pdf_viewer"]' in
tests/test_interactive_features.py and revise the comment above that assertion
so it accurately describes expect(iframe_components).to_have_count(1).
Signed-off-by: Luca Foppiano <luca@foppiano.org>
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.
Yet another try...
Summary by CodeRabbit
New Features
Tests
Chores