Repository navigation
chore: Add explicit Python CLI release workflow - #767
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:
📝 WalkthroughWalkthrough
ChangesRelease Workflow Infrastructure
WebSocket Proxy Routing
UI Artifact Type Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: c8690f2b2f
ℹ️ 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".
|
|
||
| for (const packageTarget of targets) { | ||
| const pkg = packagesByTarget[packageTarget]; | ||
| run(pkg.label, pkg.cwd, 'uvx', pkg.uploadArgs); |
There was a problem hiding this comment.
When the target includes sqlrooms-server, this upload now runs after only pnpm prerelease; in python/sqlrooms-server/package.json that prerelease script runs tests/ruff/format but does not run uv build, while the previous release script did pnpm run build immediately before twine upload. A server-only or all publish can therefore upload a stale wheel/sdist left in python/dist (or fail if none exists) instead of the just-versioned package.
Useful? React with 👍 / 👎.
ilyabo
changed the title
![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 `@scripts/publish-sqlrooms-cli.mjs`:
- Around line 203-216: The publish flow runs pnpm prerelease for all targets and
then immediately uploads via uvx, but for python/sqlrooms-server the prerelease
script does not build artifacts, causing potentially stale or missing
distributions to be uploaded. Add an explicit build step between the prerelease
loop (lines 203-205) and the upload loop (lines 213-216) that iterates through
targets again and runs a build command (such as pnpm build) for each package
before the upload attempt to ensure fresh artifacts are created.
🪄 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: 1a563bc0-ca1a-4738-9b39-25285dfacf64
📒 Files selected for processing (3)
package.jsonpython/sqlrooms/README.mdscripts/publish-sqlrooms-cli.mjs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f781139c7a
ℹ️ 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".
| } | ||
| }; | ||
|
|
||
| const publishPackages = ({target, dryRun}) => { |
There was a problem hiding this comment.
When a release operator runs pnpm cli:publish --target sqlrooms --bump patch or --set ..., parseArgs accepts those flags but this function only consumes target and dryRun, so publishing proceeds with the existing package.json versions. Because uploads use --skip-existing, this can appear to succeed while no intended new release was produced; fail fast on versioning flags for the publish command so the required explicit cli:version step cannot be silently skipped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/sqlrooms/tests/test_launcher.py (1)
54-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider verifying
crdtWsUrlas well.The test correctly validates that
wsUrluses the same-origin proxy format. SincecrdtWsUrlalso uses the same proxy URL (as shown in line 715 of launcher.py), consider adding an assertion to verify it matches the expected proxy URL as well.✅ Suggested assertion to add
assert response.status_code == 200 assert response.json()["wsUrl"] == "ws://127.0.0.1:4173/ws/duckdb" +assert response.json()["crdtWsUrl"] == "ws://127.0.0.1:4173/ws/duckdb"🤖 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/tests/test_launcher.py` around lines 54 - 67, The test_api_config_uses_same_origin_ws_proxy function currently only verifies that the wsUrl field in the API response uses the same-origin proxy format, but it should also validate crdtWsUrl since it uses the same proxy URL according to launcher.py. Add an additional assertion after the existing wsUrl assertion to verify that response.json()["crdtWsUrl"] equals the expected proxy URL "ws://127.0.0.1:4173/ws/duckdb", matching the expected format for wsUrl.apps/sqlrooms-cli-ui/README.md (1)
79-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the working directory assumption.
The
node -p "require('./python/sqlrooms/package.json').version"command assumes the current working directory is the repository root. Consider adding a brief note or changing the command to be explicit about the expected working directory.📝 Suggested clarification
Add a note before the code block:
+From the repository root: + ```bash SQLROOMS_VERSION=$(node -p "require('./python/sqlrooms/package.json').version")Or make the path absolute from repo root:
-SQLROOMS_VERSION=$(node -p "require('./python/sqlrooms/package.json').version") +SQLROOMS_VERSION=$(node -p "require(process.cwd() + '/python/sqlrooms/package.json').version")🤖 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 `@apps/sqlrooms-cli-ui/README.md` around lines 79 - 82, The README section containing the SQLROOMS_VERSION command and subsequent installation/execution steps does not document that these commands must be run from the repository root directory. Add a brief note before this code block clarifying that all commands should be executed from the repository root, or alternatively modify the relative path in the node command to be explicit about the expected working directory context.
🤖 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 `@apps/sqlrooms-cli-ui/src/artifactTypes.tsx`:
- Around line 177-178: The create command for the app artifact type in
createDashboardCommands.ts is passing 'App' as the label to
createArtifactCommand(), but the artifact type definition in artifactTypes.tsx
now specifies 'App Builder' as the label. Find the line where
createArtifactCommand is called with artifactType 'app' and the label 'App', and
change the label parameter from 'App' to 'App Builder' to match the artifact
type definition and ensure consistent labeling across the application.
---
Nitpick comments:
In `@apps/sqlrooms-cli-ui/README.md`:
- Around line 79-82: The README section containing the SQLROOMS_VERSION command
and subsequent installation/execution steps does not document that these
commands must be run from the repository root directory. Add a brief note before
this code block clarifying that all commands should be executed from the
repository root, or alternatively modify the relative path in the node command
to be explicit about the expected working directory context.
In `@python/sqlrooms/tests/test_launcher.py`:
- Around line 54-67: The test_api_config_uses_same_origin_ws_proxy function
currently only verifies that the wsUrl field in the API response uses the
same-origin proxy format, but it should also validate crdtWsUrl since it uses
the same proxy URL according to launcher.py. Add an additional assertion after
the existing wsUrl assertion to verify that response.json()["crdtWsUrl"] equals
the expected proxy URL "ws://127.0.0.1:4173/ws/duckdb", matching the expected
format for wsUrl.
🪄 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: 617c11c8-6136-4e42-93a4-a8b5b0d7d828
📒 Files selected for processing (7)
apps/sqlrooms-cli-ui/README.mdapps/sqlrooms-cli-ui/src/artifactTypes.tsxpython/sqlrooms/README.mdpython/sqlrooms/package.jsonpython/sqlrooms/sqlrooms/cli.pypython/sqlrooms/sqlrooms/web/launcher.pypython/sqlrooms/tests/test_launcher.py
💤 Files with no reviewable changes (1)
- python/sqlrooms/README.md
✅ Files skipped from review due to trivial changes (1)
- python/sqlrooms/package.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62b7719804
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62b7719804
ℹ️ 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".
| ), | ||
| canvas: withStability('canvas', canvasDefinition, experimentalEnabled), | ||
| app: withStability('app', appDefinition, experimentalEnabled), | ||
| 'app-builder': withStability( |
There was a problem hiding this comment.
For projects that already contain the experimental App Builder artifact, persisted metadata still has type: "app"; after this change only app-builder is registered, and SelectedCliArtifactContent resolves artifactTypes[artifact.type] then returns null when there is no component, so those saved artifacts open as blank content. Please add a migration or keep an app alias alongside the new ID before dropping the old key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/sqlrooms-cli-ui/src/artifactTypes.tsx`:
- Around line 228-232: The removal of the legacy 'app' artifact type
registration breaks backward compatibility with existing persisted artifacts
that have artifact.type === 'app', causing them to fall out of CLI handling and
become inaccessible. Add a temporary legacy alias or migration mapping in the
artifactTypes structure that routes 'app' type artifacts to 'app-builder' to
ensure existing artifacts remain compatible after the upgrade.
In `@apps/sqlrooms-cli-ui/src/createDashboardCommands.ts`:
- Around line 234-237: The createDashboardCommands.ts file has been updated to
change the command registration from 'app' to 'app-builder' in the object where
createArtifactCommand is called with 'app-builder' and 'App Builder', but no
corresponding test updates have been included. Add or update tests in the
appropriate test file to verify that the 'app-builder' command is properly
registered and can be invoked with the correct artifactType and display name.
Additionally, consider whether any legacy 'app' command handling should be
tested or if you need to add tests to ensure the renamed command works as
expected in the command registration layer.
🪄 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: 51742f27-7de9-4055-90ba-b9b940bb9090
📒 Files selected for processing (7)
apps/sqlrooms-cli-ui/README.mdapps/sqlrooms-cli-ui/src/artifactTypeIds.tsapps/sqlrooms-cli-ui/src/artifactTypes.tsxapps/sqlrooms-cli-ui/src/createDashboardCommands.tspython/sqlrooms/package.jsonpython/sqlrooms/tests/test_launcher.pyscripts/publish-sqlrooms-cli.mjs
✅ Files skipped from review due to trivial changes (2)
- python/sqlrooms/package.json
- apps/sqlrooms-cli-ui/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- python/sqlrooms/tests/test_launcher.py
- scripts/publish-sqlrooms-cli.mjs
| 'app-builder': withStability( | ||
| 'app-builder', | ||
| appDefinition, | ||
| experimentalEnabled, | ||
| ), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve compatibility for legacy app artifact records.
Line 228 removes the legacy app registration path. Existing persisted artifacts with artifact.type === 'app' can fall out of CLI handling (for example, sidebar filtering depends on CliArtifactType membership), which risks making older artifacts inaccessible after upgrade. Please add a migration on load or a temporary legacy alias (app -> app-builder) before fully removing app.
💡 Minimal compatibility shim example
diff --git a/apps/sqlrooms-cli-ui/src/artifactTypeIds.ts b/apps/sqlrooms-cli-ui/src/artifactTypeIds.ts
@@
'python',
'canvas',
+ 'app', // legacy persisted type (temporary compatibility)
'app-builder',
] as const;diff --git a/apps/sqlrooms-cli-ui/src/artifactTypes.tsx b/apps/sqlrooms-cli-ui/src/artifactTypes.tsx
@@
canvas: 'experimental',
+ app: 'experimental',
'app-builder': 'experimental',
@@
canvas: withStability('canvas', canvasDefinition, experimentalEnabled),
+ app: {
+ ...withStability('app', appDefinition, experimentalEnabled),
+ canCreate: false, // legacy only; keep command surface on app-builder
+ },
'app-builder': withStability(
'app-builder',
appDefinition,
experimentalEnabled,
),🤖 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 `@apps/sqlrooms-cli-ui/src/artifactTypes.tsx` around lines 228 - 232, The
removal of the legacy 'app' artifact type registration breaks backward
compatibility with existing persisted artifacts that have artifact.type ===
'app', causing them to fall out of CLI handling and become inaccessible. Add a
temporary legacy alias or migration mapping in the artifactTypes structure that
routes 'app' type artifacts to 'app-builder' to ensure existing artifacts remain
compatible after the upgrade.
| { | ||
| artifactType: 'app-builder', | ||
| command: () => createArtifactCommand('app-builder', 'App Builder'), | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add regression coverage for the renamed create command.
This changes command behavior/public surface from app to app-builder, but no related test update is included in this layer. Please add/adjust tests for command registration and invocation using app-builder (and any intended legacy handling).
As per coding guidelines, "Add or update tests when changing behavior or public API, when practical."
🤖 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 `@apps/sqlrooms-cli-ui/src/createDashboardCommands.ts` around lines 234 - 237,
The createDashboardCommands.ts file has been updated to change the command
registration from 'app' to 'app-builder' in the object where
createArtifactCommand is called with 'app-builder' and 'App Builder', but no
corresponding test updates have been included. Add or update tests in the
appropriate test file to verify that the 'app-builder' command is properly
registered and can be invoked with the correct artifactType and display name.
Additionally, consider whether any legacy 'app' command handling should be
tested or if you need to add tests to ensure the renamed command works as
expected in the command registration layer.
Source: Coding guidelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5daacb0810
ℹ️ 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".
| else None | ||
| ) | ||
| ws_url = self.external_ws_url or derived_ws_url or self._ws_url() | ||
| ws_url = self.external_ws_url or derived_ws_url or self._ws_proxy_url() |
There was a problem hiding this comment.
When the HTTP server is bound to a non-loopback address, e.g. --host 192.168.1.10, this fallback now advertises the FastAPI /ws/duckdb proxy instead of the direct local-only backend. I checked the proxy route and it does not call _require_api_auth, while _run_duckdb_server starts the upstream with auth_token=None, so any client that can reach the HTTP port can open the advertised WebSocket and issue DuckDB websocket commands despite the backend being started as local-only. Please either keep the direct local-only URL unless an external proxy is explicitly configured, or require the session token on this proxy before making it the default.
Useful? React with 👍 / 👎.
Summary
cli:version,cli:publish, andcli:publish:dryentrypoints for the Python CLI release flowpublish-clicommands working through the new wrappersTesting
Summary by CodeRabbit
sqlrooms --versionflag in the Python README.--versionoption to the Python CLI.--versionoutput and WebSocket proxy URL behavior.