Repository navigation
feat(cli): prepare sqlrooms publishing - #754
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:
📝 WalkthroughWalkthroughIntroduces an ChangesexperimentalEnabled flag, canCreate artifact contract, and sqlrooms-cli → sqlrooms rename
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: d24d9021be
ℹ️ 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".
| DOCUMENT_COMMAND_OWNER, | ||
| createDocumentCommands<RoomState>(), | ||
| ); | ||
| if (experimentalEnabled) { |
There was a problem hiding this comment.
This only gates the document command owner, but createDashboardCommands() registered just above still includes per-type create commands such as pivot.create-artifact, notebook.create-artifact, html-app.create-artifact, canvas.create-artifact, and app.create-artifact. Those commands call state.artifacts.createArtifact directly rather than the new canCreate path, so command palette or AI command execution can still create experimental artifacts when --experimental is not enabled; split or filter those commands based on experimentalEnabled.
Useful? React with 👍 / 👎.
| ): | ||
| """ | ||
| Start the SQLRooms local experience: | ||
| Launch a local SQLRooms project for adding data and building worksheets with Mosaic charts and dashboards. |
There was a problem hiding this comment.
Changing the callback docstring changes Typer's --help output to start with “Launch…”, but python/sqlrooms/tests/test_cli.py::test_cli_help still asserts the old “Start the SQLRooms local experience” text. I ran PYTHONPATH=python/sqlrooms pytest -q python/sqlrooms/tests/test_cli.py::test_cli_help, and it fails on that assertion, so the Python PR checks will fail until the test expectation or help text is updated.
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: 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 @.github/workflows/pr-checks-python.yml:
- Around line 32-38: Replace the tag-based action references in the GitHub
Actions workflow with full commit SHAs for improved supply chain security.
Specifically, update the pnpm/action-setup action (currently using `@v4`) and the
actions/setup-node action (currently using `@v6`) by replacing their version tags
with their corresponding full commit SHA hashes. This prevents the risk of tag
reassignment and ensures reproducible, immutable action references.
In `@python/sqlrooms/sqlrooms/web/launcher.py`:
- Around line 550-555: The _ui_url() method produces malformed IPv6 URLs because
IPv6 addresses must be enclosed in square brackets when combined with a port
number in a URL. Modify the _ui_url() method to check if _ui_host() returns an
IPv6 address (by detecting if it contains colons) and wrap it in square brackets
before constructing the URL, so that addresses like ::1 become [::1] in the
final URL.
🪄 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: 6e799849-0e02-410b-960e-eb6ce401d392
⛔ Files ignored due to path filters (3)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpython/sqlrooms/tests/fixtures/cars.csvis excluded by!**/*.csvpython/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
.github/workflows/pr-checks-python.ymlapps/sqlrooms-cli-ui/README.mdapps/sqlrooms-cli-ui/src/artifactTypes.tsxapps/sqlrooms-cli-ui/src/components/useLocalFileLoader.tsapps/sqlrooms-cli-ui/src/createDashboardAgent.tsapps/sqlrooms-cli-ui/src/createWorksheetAgent.tsapps/sqlrooms-cli-ui/src/layout.tsxapps/sqlrooms-cli-ui/src/runtimeConfig.tsapps/sqlrooms-cli-ui/src/statefulBlockArtifactConfigs.tsapps/sqlrooms-cli-ui/src/store.tsapps/sqlrooms-cli-ui/src/workspace/ArtifactsContainerPanel.tsxapps/sqlrooms-cli-ui/src/workspace/WorksheetArtifact.tsxdev-docs/contributing/python.mddev-docs/roadmaps/sqlrooms-cli-first-launch.mddocs/deployment-scenarios.mdpackages/artifacts/README.mdpackages/artifacts/src/ArtifactTypes.tspackages/artifacts/src/artifactWorkspace.tsxpackages/utils/__tests__/filepaths.test.tspython/README.mdpython/pyproject.tomlpython/sqlrooms-cli/package.jsonpython/sqlrooms-server/package.jsonpython/sqlrooms/.gitignorepython/sqlrooms/LICENSEpython/sqlrooms/README.mdpython/sqlrooms/clipython/sqlrooms/knip.jsonpython/sqlrooms/package.jsonpython/sqlrooms/pyproject.tomlpython/sqlrooms/scripts/dev.mjspython/sqlrooms/scripts/verify_wheel.pypython/sqlrooms/sqlrooms/cli.pypython/sqlrooms/sqlrooms/web/__init__.pypython/sqlrooms/sqlrooms/web/db_bridge/__init__.pypython/sqlrooms/sqlrooms/web/db_bridge/connectors/__init__.pypython/sqlrooms/sqlrooms/web/db_bridge/connectors/base.pypython/sqlrooms/sqlrooms/web/db_bridge/connectors/postgres.pypython/sqlrooms/sqlrooms/web/db_bridge/connectors/snowflake.pypython/sqlrooms/sqlrooms/web/db_bridge/factory.pypython/sqlrooms/sqlrooms/web/db_bridge/registry.pypython/sqlrooms/sqlrooms/web/db_bridge/types.pypython/sqlrooms/sqlrooms/web/db_bridge/utils.pypython/sqlrooms/sqlrooms/web/launcher.pypython/sqlrooms/sqlrooms/web/ui.pypython/sqlrooms/tests/__init__.pypython/sqlrooms/tests/test_cli.pypython/sqlrooms/tests/test_db_bridge.pypython/sqlrooms/tests/test_launcher.pypython/sqlrooms/tests/test_ui_provider.pyscripts/deploy-sqlrooms-cli-to-sprite.shscripts/dev.mjs
💤 Files with no reviewable changes (1)
- python/sqlrooms-cli/package.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/sqlrooms-cli-ui/src/createDashboardCommands.ts (1)
239-241: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd TSDoc for exported
createDashboardCommands.Line 239 exports a public function but it has no TSDoc; please document the
artifactTypesoption and defaultcanCreatebehavior.Proposed patch
+/** + * Builds dashboard/workspace commands and conditionally exposes per-artifact + * create commands based on artifact capability flags. + * + * `@param` options.artifactTypes Optional capability map keyed by artifact type. + * A type is creatable unless `canCreate` is explicitly `false`. + */ export function createDashboardCommands({ artifactTypes, }: CreateDashboardCommandsOptions = {}): RoomCommand<RoomState>[] {As per coding guidelines, "Add tsdoc to exported types and functions".
🤖 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 239 - 241, The exported function `createDashboardCommands` on line 239 is missing TSDoc documentation required by coding guidelines. Add a TSDoc comment block above the function definition that documents the purpose of the function, describes the `artifactTypes` parameter in the `CreateDashboardCommandsOptions` object, and explains the default `canCreate` behavior. Follow standard TypeScript JSDoc conventions with `@param` and `@returns` tags to clearly describe the function signature and its behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@apps/sqlrooms-cli-ui/src/createDashboardCommands.ts`:
- Around line 239-241: The exported function `createDashboardCommands` on line
239 is missing TSDoc documentation required by coding guidelines. Add a TSDoc
comment block above the function definition that documents the purpose of the
function, describes the `artifactTypes` parameter in the
`CreateDashboardCommandsOptions` object, and explains the default `canCreate`
behavior. Follow standard TypeScript JSDoc conventions with `@param` and `@returns`
tags to clearly describe the function signature and its behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14b470a0-6c71-4cd4-81bd-a1b80b0c9223
📒 Files selected for processing (6)
.github/workflows/pr-checks-python.ymlapps/sqlrooms-cli-ui/src/createDashboardCommands.tsapps/sqlrooms-cli-ui/src/store.tspython/sqlrooms/sqlrooms/web/launcher.pypython/sqlrooms/tests/test_cli.pypython/sqlrooms/tests/test_launcher.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/sqlrooms/tests/test_cli.py
- .github/workflows/pr-checks-python.yml
- apps/sqlrooms-cli-ui/src/store.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d935020b77
ℹ️ 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".
| onRename: undefined, | ||
| onDelete: undefined, |
There was a problem hiding this comment.
When a project already contains an experimental artifact and is reopened without --experimental, the placeholder artifact is still selectable and the topbar/sidebar still allow renaming or deleting it. Clearing these hooks means those actions update only the artifact registry: for example a pivot or html-app rename leaves the backing pivot/htmlApps title stale, and delete leaves orphaned backing state that reappears if the project is later opened with --experimental. Keep the rename/delete lifecycle hooks wired, or disable those actions for placeholder artifacts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a22dc03d6
ℹ️ 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".
| extraTools: () => ({ | ||
| embedded_html_app_agent: htmlAppAgentTool(store), | ||
| }), | ||
| extraTools: () => { |
There was a problem hiding this comment.
In non---experimental sessions this only removes embedded_html_app_agent, but createWorksheetAgentTool() still registers its built-in add_html_app_block tool, whose implementation appends a blockType: 'html-app' stateful block. For worksheet app/D3/browser-app requests the agent can therefore still create experimental worksheet HTML-app blocks while the populate tool is absent, leaving a disabled placeholder/backing state despite the feature gate; the worksheet tool set or instructions need to be filtered as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ec0924222
ℹ️ 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 dashboardAgentTool = ( | ||
| experimentalEnabled | ||
| ? createDashboardAgentToolWithDeckMaps | ||
| : createDashboardAgentTool | ||
| )({ |
There was a problem hiding this comment.
In non---experimental sessions this selects the plain Mosaic dashboard agent, but createWorksheetAgentTool still uses its default instructions/tool description that tell the worksheet agent to handle map/geospatial requests by adding or reusing a dashboard block and calling embedded_dashboard_agent. For a worksheet map request, the agent can therefore create a dashboard block and then lacks deck-map tools to populate it, leaving an empty or wrong block despite maps being feature-gated; pass a deck-map flag through the worksheet agent instructions or suppress worksheet map routing when experimentalEnabled is false.
Useful? React with 👍 / 👎.
Summary
Verification
Summary by CodeRabbit
--experimentaland--experimental-syncflags to enable preview-only experimental UI/artifacts.--syncis rejected;--experimental-syncrequires--experimental./api/confignow reportsexperimentalEnabledand no longer includes a top-level API key.sqlrooms.