Repository navigation
fix: improve CLI app serving and publishing - #755
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the ChangesSPA Routing Overhaul
CLI Database Path Validation
Debug & Logging Control
Development and Default Port Coordination
Python Package Publishing Infrastructure
UI and Product Updates
Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpServer
participant _resolve_static_file
participant _stale_entry_asset_redirect
participant FileSystem
Client->>HttpServer: GET /
HttpServer->>HttpServer: _index_response()
HttpServer-->>Client: 200 FileResponse index.html (no-store)
Client->>HttpServer: GET /assets/index-current.js
HttpServer->>_resolve_static_file: resolve "assets/index-current.js"
_resolve_static_file->>FileSystem: check static_dir/assets/index-current.js
FileSystem-->>_resolve_static_file: file exists
_resolve_static_file-->>HttpServer: resolved Path
HttpServer-->>Client: 200 FileResponse (asset content)
Client->>HttpServer: GET /assets/index-stale.js
HttpServer->>_resolve_static_file: resolve "assets/index-stale.js"
_resolve_static_file->>FileSystem: check static_dir/assets/index-stale.js
FileSystem-->>_resolve_static_file: not found
_resolve_static_file-->>HttpServer: None
HttpServer->>_stale_entry_asset_redirect: check for match
_stale_entry_asset_redirect->>FileSystem: find index-* in assets/
FileSystem-->>_stale_entry_asset_redirect: index-current.js
_stale_entry_asset_redirect-->>HttpServer: redirect target
HttpServer-->>Client: 302 RedirectResponse (no-store)
Client->>HttpServer: GET /assets/not-built.js
HttpServer->>_resolve_static_file: resolve "assets/not-built.js"
_resolve_static_file->>FileSystem: check static_dir/assets/not-built.js
FileSystem-->>_resolve_static_file: not found
_resolve_static_file-->>HttpServer: None
HttpServer->>_stale_entry_asset_redirect: check for match
_stale_entry_asset_redirect->>FileSystem: find matching index-*
FileSystem-->>_stale_entry_asset_redirect: no match found
_stale_entry_asset_redirect-->>HttpServer: None
HttpServer-->>Client: 404 JSONResponse
Client->>HttpServer: GET /api/not-a-route
HttpServer-->>Client: 404 JSONResponse error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5d116aa0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async def spa_index(): | ||
| return self._index_response() | ||
|
|
||
| @app.get("/{full_path:path}") |
There was a problem hiding this comment.
For HEAD requests to built assets (for example proxy/cache validation against /assets/index-*.js), this catch-all is registered only as GET; FastAPI does not add HEAD for @app.get, so those requests now return 405 even though the removed StaticFiles handler accepted both GET and HEAD. Register the static/index routes for HEAD as well or keep StaticFiles for those paths.
Useful? React with 👍 / 👎.
| if self.index_html.exists(): | ||
| return FileResponse(self.index_html) | ||
| return self._index_response() |
There was a problem hiding this comment.
When a GET request uses a missing or wrong-method API URL such as /api/nope or GET /api/upload, it now reaches this SPA fallback and returns index.html with HTTP 200 because the path is not under assets/. That turns API errors into successful HTML responses for clients expecting JSON, so exclude API-prefixed paths from the SPA fallback before serving the index.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/sqlrooms/sqlrooms/web/launcher.py`:
- Around line 1146-1162: In the spa_fallback function, add a check for API paths
that do not match any explicit route. Before returning the index.html response
at the end, add a condition that checks if full_path starts with "api/". If it
does, return a JSONResponse with an error message and a 404 status code (similar
to how the "assets/" check is handled). This ensures that unknown API paths like
/api/unknown return a proper JSON 404 error instead of falling through to the
SPA index response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb8a2fc5-dcef-4127-a566-694ed9b33803
📒 Files selected for processing (2)
python/sqlrooms/sqlrooms/web/launcher.pypython/sqlrooms/tests/test_launcher.py
ilyabo
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 469c51dd2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| uv run --project python --package sqlrooms sqlrooms \ | ||
| ./smoke.duckdb \ | ||
| --no-open-browser |
There was a problem hiding this comment.
I checked the generated Typer help, which reports Usage: root [OPTIONS] [DB_PATH] COMMAND [ARGS]...; with this command group, placing --no-open-browser after ./smoke.duckdb is parsed as a command and exits with No such command '--no-open-browser' instead of launching. The new smoke-test commands repeat this ordering, so users following the local/release smoke docs cannot run them; put --no-open-browser before the DB path.
Useful? React with 👍 / 👎.
…ms into cli-for-publishing-fixes
ilyabo
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c87cc2536f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.sync_enabled and self.duckdb_database == ":memory:" | ||
| ), | ||
| local_only=True, | ||
| log_startup_message=self.debug, |
There was a problem hiding this comment.
When a released sqlrooms is installed alongside sqlrooms-server 0.1.1 (still allowed by python/sqlrooms/pyproject.toml, and the server package version is unchanged), this call passes log_startup_message to the old server() signature, which did not accept that keyword in the parent commit. The exception is caught as a backend startup failure, so the UI starts degraded and DuckDB queries cannot run; either avoid the new keyword or bump sqlrooms-server and the dependency lower bound together.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/sqlrooms/sqlrooms/cli.py (1)
522-530:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate that the database path is not empty or whitespace-only.
The validation checks for
Nonebut does not verify thatresolved_db_pathis a non-empty string. Users could passsqlrooms ""or--db-path "", which would bypass this check and potentially cause confusing downstream errors.🛡️ Proposed fix to add empty string validation
resolved_db_path = db_path if db_path is not None else db_path_option -if resolved_db_path is None: +if not resolved_db_path or not resolved_db_path.strip(): typer.echo( "Please provide a DuckDB project file, e.g. `sqlrooms ./my-project.duckdb`, " "or pass `--db-path :memory:` for a temporary in-memory session.", err=True, ) raise typer.Exit(code=1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/sqlrooms/sqlrooms/cli.py` around lines 522 - 530, The validation check for resolved_db_path only verifies if it is None, but does not check for empty strings or whitespace-only strings. Modify the condition that currently checks if resolved_db_path is None to also validate that the string is not empty after stripping whitespace. Use Python's string methods like .strip() to ensure the resolved_db_path contains actual non-whitespace content, so that attempts to pass empty strings like "" or whitespace-only strings will trigger the same error message and exit as the None case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@python/sqlrooms/sqlrooms/cli.py`:
- Around line 522-530: The validation check for resolved_db_path only verifies
if it is None, but does not check for empty strings or whitespace-only strings.
Modify the condition that currently checks if resolved_db_path is None to also
validate that the string is not empty after stripping whitespace. Use Python's
string methods like .strip() to ensure the resolved_db_path contains actual
non-whitespace content, so that attempts to pass empty strings like "" or
whitespace-only strings will trigger the same error message and exit as the None
case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afe58395-982c-4723-b0d0-ce53cc9423fd
📒 Files selected for processing (9)
apps/sqlrooms-cli-ui/README.mdapps/sqlrooms-cli-ui/package.jsonapps/sqlrooms-cli-ui/vite.config.tspython/sqlrooms/README.mdpython/sqlrooms/scripts/dev.mjspython/sqlrooms/sqlrooms/cli.pypython/sqlrooms/tests/test_cli.pyscripts/deploy-sqlrooms-cli-to-sprite.shscripts/dev.mjs
✅ Files skipped from review due to trivial changes (3)
- scripts/deploy-sqlrooms-cli-to-sprite.sh
- python/sqlrooms/README.md
- apps/sqlrooms-cli-ui/README.md
Summary
Cache-Control: no-storeso the browser does not cache the entry shell.StaticFiles.Testing
Summary by CodeRabbit
Release Notes
--debugto the CLI for more verbose logging.publish-cliandpublish-cli:checkfor releasing packages.3000; database selection is now required unless--db-pathis provided.