Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances CoPaw's deployment options by introducing comprehensive desktop application packaging capabilities for both macOS and Windows. It integrates Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of scripts and code to package the CoPaw application as a desktop app for both Windows and macOS. The approach leverages conda-pack to create self-contained environments and pywebview for the user interface, which is a solid strategy. The implementation includes build scripts for both platforms, a new desktop CLI command, and supporting documentation. While the overall structure is well-conceived, I've identified critical issues in both the macOS and Windows build scripts related to the unpacking of the conda environment. These issues will prevent the packaging process from completing successfully and need to be addressed.
| if (Test-Path $Unpacked) { Remove-Item -Recurse -Force $Unpacked } | ||
| Expand-Archive -Path $Archive -DestinationPath $Unpacked -Force |
There was a problem hiding this comment.
The Expand-Archive cmdlet does not support stripping top-level directories. Since conda-pack creates an archive with a top-level directory, the environment will be unpacked into a subdirectory within $Unpacked (e.g., dist\win-unpacked\copaw_pack_...). The rest of the script incorrectly assumes that environment files like python.exe are at the root of $Unpacked, which will cause the script to fail.
To fix this, you need to move the contents of the unpacked subdirectory up to the $Unpacked directory. You can add the following logic after the Expand-Archive command:
# ... after Expand-Archive
# conda-pack creates a single directory inside the archive. Move its contents up.
$EnvDir = Get-ChildItem -Path $Unpacked -Directory | Select-Object -First 1
if ($EnvDir) {
# Move contents of the subdirectory up
Get-ChildItem -Path $EnvDir.FullName | Move-Item -Destination $Unpacked
# Remove the now-empty subdirectory
Remove-Item -Recurse -Force $EnvDir.FullName
}|
|
||
| # Unpack conda env into Resources/env | ||
| mkdir -p "${APP_DIR}/Contents/Resources/env" | ||
| tar -xzf "$ARCHIVE" -C "${APP_DIR}/Contents/Resources/env" --strip-components=0 |
There was a problem hiding this comment.
The tar command uses --strip-components=0, which is likely incorrect. conda-pack typically creates an archive with a top-level directory named after the environment. To place the environment contents directly into ${APP_DIR}/Contents/Resources/env, you should use --strip-components=1. Without this change, the paths to binaries within the .app bundle will be incorrect, causing the application to fail at launch.
| tar -xzf "$ARCHIVE" -C "${APP_DIR}/Contents/Resources/env" --strip-components=0 | |
| tar -xzf "$ARCHIVE" -C "${APP_DIR}/Contents/Resources/env" --strip-components=1 |
There was a problem hiding this comment.
Pull request overview
Adds a “desktop” distribution path for CoPaw by introducing a new CLI subcommand that launches the FastAPI app on a free port and opens it in a native webview window, plus cross-platform packaging scripts and a release workflow to build desktop artifacts.
Changes:
- Add
copaw desktopCLI command and register it in the main CLI entrypoint. - Add Windows/macOS packaging scripts (conda-pack based) and assets/docs to build distributable desktop bundles/installers.
- Add a GitHub Actions workflow to build and upload Windows/macOS desktop artifacts on release/manual dispatch.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/copaw/cli/main.py | Registers the new desktop command and records its import timing. |
| src/copaw/cli/desktop_cmd.py | Implements copaw desktop (spawn uvicorn + open pywebview window). |
| scripts/pack/gen_icon_icns.py | Generates a macOS .icns icon from the repo SVG asset. |
| scripts/pack/copaw_desktop.nsi | NSIS script to package the unpacked Windows build into an installer. |
| scripts/pack/build_win.ps1 | Windows one-click build: console build → conda-pack → unpack → NSIS installer. |
| scripts/pack/build_macos.sh | macOS one-click build: console build → conda-pack → .app bundle (+ optional zip/dmg). |
| scripts/pack/build_common.py | Shared helper to create a temp conda env, install CoPaw, and run conda-pack. |
| scripts/pack/assets/icon.svg | Adds a desktop app icon SVG source. |
| scripts/pack/README_zh.md | Chinese documentation for desktop packaging scripts and CI workflow. |
| scripts/pack/README.md | English documentation for desktop packaging scripts and CI workflow. |
| pyproject.toml | Adds pywebview dependency and introduces a full optional-dependency extra. |
| .github/workflows/desktop-release.yml | Workflow to build Windows installer + macOS zip and upload to artifacts/release assets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| proc.wait() | ||
| except KeyboardInterrupt: | ||
| proc.terminate() | ||
| proc.wait() |
There was a problem hiding this comment.
On server startup timeout, the code prints an error but then calls proc.wait() without terminating the subprocess. That can leave the command hanging indefinitely while the server continues running, which is surprising after a "did not become ready" message. Consider terminating the subprocess and exiting non-zero (or explicitly documenting/implementing a mode that keeps the server running and opens the URL automatically).
| proc.wait() | |
| except KeyboardInterrupt: | |
| proc.terminate() | |
| proc.wait() | |
| proc.terminate() | |
| proc.wait() | |
| except KeyboardInterrupt: | |
| proc.kill() | |
| proc.wait() | |
| sys.exit(1) |
| def _wait_for_http(host: str, port: int, timeout_sec: float = 15.0) -> bool: | ||
| """Return True when something accepts TCP on host:port.""" |
There was a problem hiding this comment.
Function name _wait_for_http doesn't match its behavior/docstring: it only waits for a TCP connect, not an HTTP response. Rename to something like _wait_for_tcp (or implement a real HTTP health check) to avoid misleading semantics.
| finally: | ||
| _run(["conda", "env", "remove", "-n", env_name, "-y"]) |
There was a problem hiding this comment.
The finally block always runs conda env remove ... with check=True. If env creation fails partway through, this cleanup step can fail and mask the original error. Track whether the env was successfully created (or ignore failures during cleanup) so build failures report the real root cause and cleanup is best-effort.
| "lark-oapi>=1.5.3", | ||
| "python-telegram-bot>=20.0", | ||
| "twilio>=9.10.2", | ||
| "pywebview>=4.0", | ||
| ] |
There was a problem hiding this comment.
pywebview is added to the base project.dependencies, which forces all copaw installs (including headless/server deployments) to pull in a GUI/webview dependency that can be platform-specific and fail to build/run. This should be moved into a dedicated optional extra (e.g. desktop) and the CLI should reference that extra consistently.
| full = [ | ||
| "copaw[local,ollama,llamacpp,mlx]", | ||
| ] |
There was a problem hiding this comment.
The full extra depends on copaw[...] (the same distribution), which creates a self-referential requirement that can confuse dependency resolvers and sometimes leads to resolution loops. Prefer expanding full to the concrete dependency list (or using setuptools' recommended pattern for an "all" extra without referencing the project itself).
| if webview is None: | ||
| click.echo( | ||
| "Webview support not installed. Run: pip install copaw[desktop]", | ||
| err=True, | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
This command uses sys.exit(1) inside a Click command. In this CLI codebase, other commands typically raise click.ClickException (or use Click's error/abort mechanisms), which results in consistent formatting and easier testing. Consider raising a Click exception with the same message and returning a non-zero exit code via Click instead of calling sys.exit directly.
| if _wait_for_http(host, port): | ||
| webview.create_window( | ||
| "CoPaw Desktop", | ||
| url, | ||
| width=1280, | ||
| height=800, | ||
| ) | ||
| webview.start() # blocks until user closes the window | ||
| proc.terminate() | ||
| proc.wait() | ||
| return # normal exit after user closed window |
There was a problem hiding this comment.
webview.start() can raise (e.g., backend init failure). If that happens, the uvicorn subprocess will keep running and the with Popen(...) context will block on exit waiting for it, potentially hanging the CLI. Wrap the webview lifecycle in a try/finally that terminates/kills the child process (with a timeout + kill() fallback) so the subprocess is always cleaned up on errors.
Description
[Describe what this PR does and why]
Related Issue: Fixes #(issue_number) or Relates to #(issue_number)
Security Considerations: [If applicable, e.g. channel auth, env/config handling]
Type of Change
Component(s) Affected
Checklist
pre-commit run --all-fileslocally and it passespytestor as relevant) and they passTesting
[How to test these changes]
Local Verification Evidence
Additional Notes
[Optional: any other context]