Uh oh!
There was an error while loading. Please reload this page.
fix: resolve NiceGUI drawer JavaScript timeout on run results - #299
fix: resolve NiceGUI drawer JavaScript timeout on run results#299olivermeyer wants to merge 1 commit into
Conversation
Claude encountered an error —— View job I'll analyze this and get back to you. |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
6ac4e2a to
7be6b46CompareClaude finished @olivermeyer's task —— View job PR #299 Review: JavaScript Timeout Fix ✅SummaryThis PR successfully fixes the NiceGUI drawer JavaScript timeout issue by reordering the initialization sequence. The implementation is architecturally sound and follows NiceGUI best practices. ✅ What's Good1. Root Cause FixThe PR correctly identifies and fixes the core issue: the drawer's JavaScript query was timing out (1s limit) when blocked by API calls. By creating the frame first with a loading state, the drawer initializes immediately. 2. Clean Implementation
3. Good UX Improvements
4. Follows Repository Standards
|
7be6b46 to
988d239CompareClaude finished @olivermeyer's task —— View job PR #299 Review: JavaScript Timeout FixI've completed my review of the changes to fix the NiceGUI drawer JavaScript timeout issue. Todo List
🎯 SummaryThis PR successfully addresses the NiceGUI drawer JavaScript timeout issue by reordering the initialization sequence. The root cause fix is architecturally sound and follows NiceGUI best practices. However, there is one critical blocking issue that must be resolved before merge. ❌ BLOCKING ISSUES (Must Fix)1. Test Failure - Text Assertion MismatchLocation: Issue: The test What Changed: # OLD: Frame created after run_data loaded, title was full run_idawait_frame(navigation_title=f"Run {run_id}", ...)
# NEW: Frame created immediately with loading stateawait_frame(navigation_title=f"Loading run {run_id[:8]}...", ...) # Line 77# Then later, JavaScript updates header to run details (lines 140-151)new_title=f"Run of {run_data.application_id} ({run_data.version_number}) on {date}"Test Expectation (line 501): awaituser.should_see(f"Run {run_with_few_items.run_id}", retries=100)Why It Fails: The test is looking for
The full run ID is never displayed in the header anymore. Required Fix: Option A (Recommended): Update test to wait for loading completion by checking for a stable element: # Navigate to the run pageawaituser.open(f"/application/run/{run_with_few_items.run_id}")
# Wait for the expansion panel to be created (line 105 in implementation)# This element only appears after run_data is loadedawaituser.should_see(marker="CODE_RUN_METADATA", retries=100)
# Alternative: Wait for the "Run" expansion text which appears after loading# await user.should_see("Run", retries=100)Option B: Update test to match the new header format: # This is less stable because the exact text depends on app_id/version/dateawaituser.should_see(f"Run of {HETA_APPLICATION_ID}", retries=100)Reproduce: uv run pytest tests/aignostics/application/gui_test.py::test_gui_run_results_pagination_show_more_button_hidden_when_few_results -vVerify Fix: make lint && make test_integration2. Test Coverage - 56% vs 80% RequiredIssue: SonarQube reports 56% coverage on new code (requires ≥80%) Root Cause: The failing test prevents coverage data from being collected for the success path in the new loading sequence. Lines Likely Uncovered (need verification after test fix):
Action Required:
✅ What's Excellent1. Root Cause Analysis & FixThe PR correctly identifies the problem: NiceGUI drawer's JavaScript query times out (1s limit) when blocked by slow API calls. The fix is elegant:
This follows NiceGUI best practices for handling async operations that might block the UI thread. 2. Clean Implementation
3. Maintains Code Architecture
4. Good UX
|
988d239 to
23d8b0fCompareClaude finished @olivermeyer's task —— View job PR #299 Review: JavaScript Timeout FixI've completed my comprehensive review of the changes to fix the NiceGUI drawer JavaScript timeout issue. Todo List
🎯 Executive SummaryThis PR successfully addresses a NiceGUI drawer JavaScript timeout issue with a well-implemented fix. The root cause is correctly identified and the solution follows NiceGUI best practices. However, there is ONE CRITICAL blocking issue that must be resolved before merge. Status: ❌ BLOCKING - Test failure must be fixed ❌ BLOCKING ISSUE (Must Fix Before Merge)Test Failure - Text Assertion MismatchLocation: Test: Root Cause: The test expects to see What Changed: # OLD: Frame created after run_data loaded, title was full run_idawait_frame(navigation_title=f"Run {run_id}", ...)
# NEW: Frame created immediately with loading state await_frame(navigation_title=f"Loading run {run_id[:8]}...", ...) # Line 77# Then later, JavaScript updates header to:new_title=f"Run of {run_data.application_id} ({run_data.version_number}) on {date}"# Example: "Run of he-tme (1.0.0-sl.4.4.0) on 12-01 14:35"Test Expectation (line 501): awaituser.should_see(f"Run {run_with_few_items.run_id}", retries=100)Why It Fails:
✅ Required FixUpdate the test assertion to wait for a stable element that appears after loading completes. Option A (Recommended - Most Robust): # Line 500-501, replace:awaituser.open(f"/application/run/{run_with_few_items.run_id}")
awaituser.should_see(f"Run {run_with_few_items.run_id}", retries=100)
# With:awaituser.open(f"/application/run/{run_with_few_items.run_id}")
# Wait for the expansion panel to be created (only appears after run_data loads)awaituser.should_see(marker="CODE_RUN_METADATA", retries=100)Why this works: The Option B (Alternative - Wait for New Header Format): # Line 501, replace:awaituser.should_see(f"Run {run_with_few_items.run_id}", retries=100)
# With:awaituser.should_see("Run of he-tme", retries=100) # Matches new header formatWhy this is less ideal: The exact application name and version may vary, making this test less flexible. Verify the Fix# 1. Apply the fix to tests/aignostics/application/gui_test.py# 2. Run the specific failing test
uv run pytest tests/aignostics/application/gui_test.py::test_gui_run_results_pagination_show_more_button_hidden_when_few_results -v
# 3. Run all GUI integration tests
uv run pytest tests/aignostics/application/gui_test.py -m integration -v
# 4. Verify linting still passes
make lint
# 5. Check coverage (should improve after test passes)
uv run pytest tests/aignostics/application/ --cov=src/aignostics/application/_gui --cov-report=term-missingAdditional Tests That May Need UpdatesI found 4 other tests that look for run-related text:
Action Required: After fixing line 501, verify lines 386 and 556 don't have the same issue.
|
|


Currently we first get run results, and only then render the GUI frame. If the former takes longer than 1s, we get a JS timeout error. After this change, we first render the frame and then get run results and populate the frame.