Skip to content

fix: resolve NiceGUI drawer JavaScript timeout on run results#305

Merged
olivermeyer merged 1 commit intomainfrom
fix/javascript-timeout-2
Dec 2, 2025
Merged

fix: resolve NiceGUI drawer JavaScript timeout on run results#305
olivermeyer merged 1 commit intomainfrom
fix/javascript-timeout-2

Conversation

@olivermeyer
Copy link
Collaborator

@olivermeyer olivermeyer commented Dec 2, 2025

Loading run data asynchronously to avoid blocking the JavaScript event loop. This should prevent timeouts and resulting GUI freezes.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2025

Claude finished @olivermeyer's task —— View job


PR Review: fix: get run details non-blocking ✅

Summary

This PR fixes a JavaScript timeout issue in the NiceGUI application run details page by converting a blocking run.details() call to a non-blocking async call using nicegui_run.io_bound(). The fix is correct, minimal, and follows established patterns in the codebase.


✅ What's Good

1. Correct Async Pattern

The change properly uses NiceGUI's nicegui_run.io_bound() to execute the blocking API call in a background thread, preventing UI freezes and JavaScript timeouts.

Pattern verified in codebase:

  • Line 72: run = await nicegui_run.io_bound(service.application_run, run_id)
  • Line 73: run_data = await nicegui_run.io_bound(run.details) ✅ (this PR)

2. Improved UX

Moving the API call before hiding the spinner (line 74) ensures users see the loading indicator during the network request, which is better UX.

3. Minimal Change

Single-line fix with surgical precision - exactly what's needed to resolve the timeout issue.

4. Conventional Commit Format

Commit message follows the required format: fix: get run details non-blocking

5. Test Coverage Exists

The modified function _page_application_run_describe() is exercised by multiple existing e2e tests:

  • test_gui_cli_submit_to_run_result_delete (line 88-181)
  • test_gui_download_dataset_via_application_to_run_cancel_to_find_back (line 188-343)
  • test_gui_run_download (line 350-458)
  • Plus integration tests for pagination (line 468-577)

📋 Architecture Compliance

Modulith Pattern Respected

  • GUI layer (_gui/) properly depends on Service layer (_service.py)
  • No circular dependencies introduced
  • Uses standard NiceGUI async patterns

Medical Device Software Standards

  • No sensitive data handling changes
  • No breaking API changes
  • UI responsiveness improvement aligns with medical device usability requirements

🔍 Code Quality Checks

Type Safety

The change maintains proper type hints:

run_data = await nicegui_run.io_bound(run.details) if run else None
# Type: RunDetails | None

Error Handling

Existing error handling at line 104-106 still applies:

if run is None:
    ui.label(f"Failed to get run '{run_id}'").mark("LABEL_ERROR")
    return

Consistent Pattern

Verified that similar calls in the codebase use the same async pattern:

  • CLI/Service layer: Uses blocking run.details() (correct - not async context)
  • GUI layer: Should use await nicegui_run.io_bound(run.details) (this PR fixes it)

🎯 Recommendations

1. Consider PR Description (Minor - Non-blocking)

The PR has no description. While the change is self-explanatory from the code, consider adding:

## Problem
JavaScript timeout in GUI when loading run details page due to blocking API call.

## Solution
Convert `run.details()` to `await nicegui_run.io_bound(run.details)` to prevent UI thread blocking.

## Testing
Covered by existing e2e tests: `test_gui_cli_submit_to_run_result_delete` and others.

2. Verify Linting Passes (Action Required)

Please run locally before merge:

make lint

This ensures:

  • Ruff formatting ✓
  • Ruff linting ✓
  • MyPy (strict) ✓
  • PyRight (basic) ✓

3. Consider Adding Label (Optional)

Since GUI tests can be flaky and time-consuming, consider adding the skip:test_long_running label to speed up CI if the long-running e2e tests aren't critical for this change.


🚀 Verdict

Status: ✅ APPROVED (pending lint verification)

Blocking Issues: None

Suggestions:

  1. Verify make lint passes locally
  2. Consider adding PR description (optional but helpful)

This is a high-quality fix that:

  • ✅ Solves the JavaScript timeout issue
  • ✅ Follows NiceGUI async best practices
  • ✅ Maintains type safety and error handling
  • ✅ Has existing test coverage
  • ✅ Uses conventional commit format
  • ✅ Respects modulith architecture
  • ✅ Improves user experience (spinner visible during load)

Great work, Oliver! This is exactly the kind of surgical fix that medical device software requires - minimal, targeted, and safe.


@codecov
Copy link

codecov bot commented Dec 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Files with missing lines Coverage Δ
...application/_gui/_page_application_run_describe.py 67.95% <100.00%> (ø)

... and 7 files with indirect coverage changes

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 2, 2025

@olivermeyer olivermeyer changed the title fix: get run details non-blocking fix: resolve NiceGUI drawer JavaScript timeout on run results Dec 2, 2025
@olivermeyer olivermeyer marked this pull request as ready for review December 2, 2025 13:29
@olivermeyer olivermeyer merged commit 51f7434 into main Dec 2, 2025
27 of 28 checks passed
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