[SYNPY-1636] Synapse Desktop Client MVP using ElectronJS and the SynapsePythonClient #1236
Conversation
…ove Windows compatibility
…ctivation in build workflow
…d directories, suppressing progress bars in packaged apps, and improving logging for better debugging.
…le names and simplify usage instructions.
| fi | ||
| ;; | ||
| "linux") | ||
| build_python_backend "linux" |
There was a problem hiding this comment.
What does this app look like just within the terminal?
The reason I ask is because there is another use case for people that work in HPCs that may just want to easily install the CLI and work within the terminal
There was a problem hiding this comment.
I don't think that this will work within the terminal - It wasn't built to be able to work without a UI. I left this is for us to target later if it's something we want to build for this platform.
In an EC2 instance when I build with the shell script I get Synapse Desktop Client-1.0.0.AppImage and running it fails with this error:
$ ./"Synapse Desktop Client-1.0.0.AppImage"
fuse: failed to exec fusermount: No such file or directory
Cannot mount AppImage, please check your FUSE setup.
You might still be able to extract the contents of this AppImage
if you run it with the --appimage-extract option.
See https://github.com/AppImage/AppImageKit/wiki/FUSE
for more information
open dir error: No such file or directory
I don't have an ubuntu instance set up with a GUI at the moment so I wasn't going to worry about it.
For the usecase of running this within HPCs and other terminals we may want to provide our CLI as a package that can be installed - Rather than this desktop application. Since the CLI will have commands meant to run in the terminal it'll lend itself much better to this usecase.
There was a problem hiding this comment.
Is there potentially duplicated code here per the "functional" class that we could provide within the client itself?
There was a problem hiding this comment.
There is some duplicated logic around getting the right file/folder structure, but this is more of an artifact of needing to convert the UI input into something that the synapseclient library can use. I used GitHub Copilot to build out the UI elements, and it ended up passing data to the backend in a more complex way that requires additional conversion.
Another area for improvement is the progress updates during operations. The synapseclient library uses TQDM progress bars to indicate progress, however, the way this process is set up, the TQDM progress bars don't work properly. The Console Log section in the desktop client isn't the actual stdout that TQDM writes to. I actually had to disable the TQDM bars entirely, otherwise the backend server would crash.
| logger.warning("Failed to setup Electron environment: %s", e) | ||
|
|
||
|
|
||
| def _detect_electron_environment() -> bool: |
There was a problem hiding this comment.
I haven't looked at the code very closely, but do we have something that says the desktop client is only supported for X system upon launching? (can be a separate ticket)
There was a problem hiding this comment.
I haven't explored this. I am making the assumption that the install process that electron automatically includes will handle for this by checking the system pre-reqs prior to install.
I am not planning to distribute the "portable" versions of the software - Although that's something we could include later on.
There was a problem hiding this comment.
Nit: typing hinting + doc string formats can be improved (but maybe for the future, let's see the traction and usage on this product)
synapse-electron/main.js
Outdated
There was a problem hiding this comment.
I'm not a JS expert, but this looks pretty straight forward to me.
| # Check if Node.js is installed | ||
| if ! command -v node &> /dev/null; then | ||
| echo "ERROR: Node.js is not installed or not in PATH" | ||
| echo "Please install Node.js from https://nodejs.org/" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Check if Python is installed | ||
| if ! command -v python3 &> /dev/null; then | ||
| if ! command -v python &> /dev/null; then | ||
| echo "ERROR: Python is not installed or not in PATH" | ||
| echo "Please install Python from https://python.org/" | ||
| exit 1 | ||
| fi | ||
| PYTHON_CMD="python" | ||
| else | ||
| PYTHON_CMD="python3" | ||
| fi |
There was a problem hiding this comment.
I think these usually come with the system, but looking at this, it does seem like if someone's laptop didn't have these installed, they would still have to install it correct? The good news is that I think both of these have executable installers
There was a problem hiding this comment.
This start script is for developers wanting to start the UI and the backend python server.
I added onto the readme in eb975f4#diff-d72d0d6be34ec014bf335064947f040cd63ba05fe7ef488c0ab75512f0dca507 to talk a bit more about this.
In order to run the UI developers will need to ensure that node and python are installed.
Users running the packaged desktop application should not need these since pyinstaller and the electronjs builder will package everything up.
There was a problem hiding this comment.
🔥 🔥 🔥 This is awesome work here. I did a couple once overs at the code and didn't looked at the core service logic and didn't find any glaring issues.
As this is really an MVP and we want to first promote then assess the usage before pouring a ton more work into this, but I'm super excited about this.
Feel free to add to the front page of the synapse client docs to promote this MVP and we can start tracking how many people download and use it.
…gging guidance; refactor logging in Synapse service for better error handling and async support
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| detail_callback=None, | ||
| ) | ||
|
|
||
| return await _handle_bulk_download_result(result, file_items_data) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
The best way to fix this problem is to avoid returning raw exception messages to the user. Concretely:
- In
bulk_download, theexcept Exception as eblock currently logs the exception and then raises an HTTPException withdetail=str(e). This should be changed to respond with a generic error string (e.g., "An internal error has occurred, please try again later."), not the actual exception message. The original exception should still be logged server-side. - In
_handle_bulk_download_result, the code raises an HTTPException withdetail=result["error"](whereresult["error"]may have originated fromstr(e)as discussed above). This should respond to the client with a generic error message as well, notresult["error"]directly. - Broadcasting user-facing progress/status messages (e.g., via
broadcast_message) should also avoid leaking internal exception strings—error fields sent to clients here should be generic.
Implementation Plan:
- In both places (
bulk_downloadexcept block and_handle_bulk_download_resulterror branch), replace direct references to exception-derived error strings with a generic message such as "Bulk download failed due to an internal server error." - Server-side logs (already present) remain unchanged for debugging.
- Optionally, you may want to pass an error "code" or reference string to the user for support, but that's not required unless already a pattern.
| @@ -693,7 +693,10 @@ | ||
| raise | ||
| except Exception as e: | ||
| logger.exception("Bulk download endpoint error") | ||
| raise HTTPException(status_code=500, detail=str(e)) from e | ||
| raise HTTPException( | ||
| status_code=500, | ||
| detail="Bulk download failed due to an internal server error.", | ||
| ) from e | ||
|
|
||
|
|
||
| def _filter_file_items(items: List[Dict[str, Any]]) -> List[Dict[str, Any]]: | ||
| @@ -795,10 +798,15 @@ | ||
| "type": "complete", | ||
| "operation": "bulk-download", | ||
| "success": False, | ||
| "data": {"error": result["error"]}, | ||
| "data": { | ||
| "error": "Bulk download failed due to an internal server error." | ||
| }, | ||
| } | ||
| ) | ||
| raise HTTPException(status_code=500, detail=result["error"]) | ||
| raise HTTPException( | ||
| status_code=500, | ||
| detail="Bulk download failed due to an internal server error.", | ||
| ) | ||
|
|
||
|
|
||
| @app.post("/bulk/upload") |
BryanFauble
changed the title
There was a problem hiding this comment.
This is a cool project that could contribute to increase our user base and enhance the UX of existing users. Below are comments and suggestions about the architecture, security, and user experience.
- The architecture could be improved using API-spec first development: a small OpenAPI description could be created for the app backend, which would then be used to generate 1) a FastAPI server stub and 2) a Node.js client library that the frontend app can consume.
- Add a small FastAPI test suite (startup, auth handshake mock, a no-op endpoint, and WebSocket echo) to prevent regressions.
- In CI, launch backend + headless Electron to run smoke tests (e.g., verify login flow against a Synapse sandbox or mocked endpoints).
- Set explicit CORS allowlist for the renderer for increased security.
- Ensure that app dependencies and tool versions used to build the artifacts are pinned to stabilize builds and reproducibility.
- We likely should ask the user to accept a user agreement or TOS. The Governance team can provide guidance.
- Document a clear update strategy. How does it depend on updates to the Synapse Python client? How does the user update the app?
- Add signing/notarization steps to prevent users from seeing warnings.
- Use Keytar or platform keychain for token storage; never store tokens in JSON/config files. This applies more directly to the Synapse Python client.
- Are the app processes well isolated or are there additional steps that should be taken?
- This project is likely too big to be stored in a library repository (Synpase Pythn client). The Sage monorepo would be a better place to develop this project.
There was a problem hiding this comment.
Looks good to me! I only have some minor comments related to documentation and type hints.
Probably should be created in a separate ticket: do we want to provide a way for users to download the desktop client directly from the documentation? And do we already have instructions for using it? For example, guidance on unblocking the application in system settings. Also, should we automatically export traces from the executable to Signoz so that we could better troubleshoot if users run into an error?
| os.makedirs(synapse_cache, exist_ok=True) | ||
| os.environ["SYNAPSE_CACHE"] = synapse_cache | ||
|
|
||
| logger.info("Set SYNAPSE_CACHE to: %s", synapse_cache) |
There was a problem hiding this comment.
Nit: while this works, I am curious do we want to be consistent in the code base to just use f string
There was a problem hiding this comment.
Technically lazy logging is the correct way to doing this https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/logging-format-interpolation.html - I know there we have a lot of cases where we use the f-strings as well, but I think I'd be ok if we use either or.
| @@ -0,0 +1,286 @@ | |||
| """ | |||
There was a problem hiding this comment.
Do we want to add tests for all these functions here?
There was a problem hiding this comment.
I created https://sagebionetworks.jira.com/browse/SYNPY-1647 for this
|
|
||
| The ElectronJS application provides access to Chromium's developer tools: | ||
|
|
||
| 1. **Open Developer Console**: |
There was a problem hiding this comment.
I didn't need to open the developer console, and it just opened for me automatically.
There was a problem hiding this comment.
I set a flag for it to open automatically when it isn't packaged, but I also wanted to explicitly call it out as something that can be done.
…cumentation - Updated type hints in config_service.py and synapse_service.py for better clarity. - Enhanced docstrings across multiple methods to provide detailed descriptions of arguments, return types, and exceptions. - Removed unused async utility function create_async_operation_wrapper from async_utils.py. - Improved logging utility functions with comprehensive docstrings and error handling.
|
Thanks @tschaffter
Maybe this is something that we could collab on when we move this to the Sage Monorepo if we get more traction to use the desktop client. Added to https://sagebionetworks.jira.com/browse/SYNPY-1649
I created https://sagebionetworks.jira.com/browse/SYNPY-1647 for this
Created https://sagebionetworks.jira.com/browse/SYNPY-1647 for this
I added this in f5ddfb7
When we move to the monorepo this would be great to enforce. I included it in https://sagebionetworks.jira.com/browse/SYNPY-1649
Added this to: I created https://sagebionetworks.jira.com/browse/SYNPY-1649 for this
Included in https://sagebionetworks.jira.com/browse/SYNPY-1646
I have these 2 tickets open for this:
I created https://sagebionetworks.jira.com/browse/SYNPY-1648 for this - We need to resolve it in the underlying Synapse Python Client first.
I have not found issues here - We will need to monitor feedback that we get.
I created https://sagebionetworks.jira.com/browse/SYNPY-1649 for this |
…ure-specific builds for macOS and Windows
…rchitecture-specific builds for macOS and Windows" This reverts commit 4410a14.
… artifact naming and remove unused macOS build specifications
Start review at the readme: https://github.com/Sage-Bionetworks/synapsePythonClient/blob/synpy-1636-move-to-electron/synapse-electron/README.md
Problem:
Solution:
Testing: