Conversation
This update adds cross-platform smoke tests to the CI workflow for macOS, Windows, and Linux to verify that bundled apps start correctly and log output. The PyInstaller spec is updated to include torch.cuda and related hidden imports for proper bundling.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes critical startup crashes on Windows and macOS for the bundled application. The root cause was missing PyTorch CUDA modules in the PyInstaller bundle and improper multiprocessing handling for frozen applications. The fix ensures proper bundling of all required torch modules, adds early multiprocessing child process detection, and implements comprehensive logging for debugging.
Key changes:
- Added multiprocessing freeze support and early child process detection to prevent GUI windows from spawning in worker processes
- Enhanced PyInstaller spec to bundle torch.cuda and torch.backends modules explicitly for all platforms
- Added platform-specific file logging and Windows error dialogs for better debugging of startup issues
- Fixed CSS hover effect on Search button to prevent layout jumping
- Implemented comprehensive smoke tests for macOS, Windows, and Linux in CI workflow
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/docfinder/gui.py | Added multiprocessing freeze_support, child process detection, platform-specific file logging, and Windows error dialogs |
| DocFinder.spec | Added torch.cuda/backends modules collection and hidden imports; updated version to 1.1.2 |
| src/docfinder/web/templates/index.html | Fixed Search button hover effect to prevent translateY transform |
| src/docfinder/web/app.py | Moved cleanup endpoint definition earlier in file for better organization |
| tests/test_web_app.py | Added comprehensive test coverage for web API endpoints |
| tests/test_frontend.py | Added tests for frontend template loading and routing |
| tests/test_cli.py | Added comprehensive CLI command tests |
| .github/workflows/build-desktop.yml | Added smoke tests for all platforms and push/pull_request triggers for early testing |
| CHANGELOG.md | Documented all changes for version 1.1.2 release |
| src/docfinder/init.py | Updated version to 1.1.2 |
| pyproject.toml | Updated version to 1.1.2 |
Comments suppressed due to low confidence (1)
tests/test_web_app.py:410
- This test creates a directory in the user's home directory. If the test fails before reaching the cleanup in the
finallyblock, the directory could be left behind. Consider using thetmp_pathfixture or adding additional cleanup logic to ensure the directory is always removed.
"updated": 0,
"skipped": 0,
"failed": 0,
"processed_files": [],
}
response = client.post("/index", json={"paths": [str(test_dir)]})
assert response.status_code == 200
assert response.json()["status"] == "ok"
finally:
test_dir.rmdir()
class TestFrontendRouter:
"""Tests for the frontend HTML routes."""
def test_index_page(self) -> None:
"""Returns HTML for index page."""
response = client.get("/")
assert response.status_code == 200
assert "text/html" in response.headers["content-type"]
assert "DocFinder" in response.text
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -212,7 +252,7 @@ if sys.platform == "darwin": | |||
| "CFBundleName": "DocFinder", | |||
| "CFBundleDisplayName": "DocFinder", | |||
| "CFBundleVersion": "1.1.1", | |||
There was a problem hiding this comment.
There's a version mismatch: CFBundleVersion is set to "1.1.1" but CFBundleShortVersionString is set to "1.1.2". Both should be "1.1.2" to match the version being released in this PR.
| "CFBundleVersion": "1.1.1", | |
| "CFBundleVersion": "1.1.2", |
|
|
||
| # Include torch subpackages explicitly (required by transformers dynamic imports) | ||
| import torch | ||
| import os |
There was a problem hiding this comment.
The os module is imported twice in this file - once at line 13 at the top of the file, and again here at line 48. Remove this redundant import statement.
| import os |
| """Get path to log file for debugging startup issues.""" | ||
| if sys.platform == "win32": | ||
| # On Windows, use LOCALAPPDATA for logs | ||
| import os |
There was a problem hiding this comment.
The os module is imported twice - once at the module level (line 27) and again here inside the function (line 51). The module-level import should be sufficient, so remove this redundant import.
| import os |
| def test_index_path_outside_home(self, tmp_path: Path) -> None: | ||
| """Returns 403 for path outside home directory.""" | ||
| # Try to access /etc which is definitely outside home | ||
| response = client.post("/index", json={"paths": ["/etc"]}) | ||
| assert response.status_code == 403 | ||
| assert "outside allowed directory" in response.json()["detail"] | ||
|
|
||
| @patch("docfinder.web.app._run_index_job") | ||
| def test_index_success( | ||
| self, mock_run_index: MagicMock, tmp_path: Path, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: |
There was a problem hiding this comment.
The tests create files directly in the user's home directory, which could potentially interfere with existing files or leave test artifacts if the test fails. Consider using the provided tmp_path fixture instead to create test files in isolated temporary directories. This would make tests more robust and avoid polluting the user's home directory.
| pass | ||
| raise SystemExit(1) |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| pass | |
| raise SystemExit(1) | |
| # Ignore all exceptions here: if showing the error message box fails, |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Description
This PR fixes the silent crash on Windows and macOS when starting the bundled application. The root cause was missing
torch.cudamodule in PyInstaller bundle and lack of propermultiprocessing.freeze_support()handling for frozen apps.Type of Change
Related Issues
Fixes #18
Changes Made
multiprocessing.freeze_support()for PyInstaller-bundled apps compatibilitytorch.cudaand related hidden imports to PyInstaller spec for proper bundlingcollect_all("torch")to ensure all torch submodules are includedpushandpull_requesttriggers to build workflow for early testingTesting
Test Coverage
Checklist
ruff checkandruff format)Screenshots (if applicable)
N/A - Bug fix for startup crash
Additional Context
The bug was reported by a Windows 11 25H2 user who experienced the app terminating silently within a minute of launch, with no visible window appearing. The fix ensures:
Deployment Notes
No special deployment considerations. The fix is backward compatible.