Repository navigation
feat: Update cli UI - #699
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds artifact-owned AI session handling, refactors the CLI workspace into sidebar-based components, updates chat/session APIs, and removes richText block support from documents. ChangesArtifact-owned AI and session contracts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
…oms into update-cli-ui
![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: 21f86e3dc3
ℹ️ 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".
| if (currentSessionArtifactId !== currentArtifactId) { | ||
| state.artifactAi.selectLatestSessionForArtifact(currentArtifactId); | ||
| } |
There was a problem hiding this comment.
When a session has no aiSessionArtifacts entry, such as sessions persisted before this commit or sessions created through the still-registered ai.create-session command that calls ai.createSession directly, currentSessionArtifactId is undefined while currentArtifactId is normally set. This branch immediately calls selectLatestSessionForArtifact, which clears or switches away from that session; because the CLI history view also filters by the mapping, those chats become inaccessible. Please backfill/map unscoped sessions or allow missing mappings instead of treating them as a mismatch.
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: 3
🧹 Nitpick comments (1)
apps/sqlrooms-cli-ui/src/components/useLocalFileLoader.ts (1)
6-11: ⚡ Quick winNon-standard MIME types in LOCAL_DATA_ACCEPTED_FORMATS.
The MIME types
text/parquetandtext/jsonare non-standard. According to IANA standards:
- Parquet files typically use
application/vnd.apache.parquetorapplication/octet-stream- JSON files use
application/jsonWhile the file input's
acceptattribute is generally forgiving, using correct MIME types improves clarity and future compatibility.📝 Proposed fix
export const LOCAL_DATA_ACCEPTED_FORMATS = { 'text/csv': ['.csv'], 'text/tsv': ['.tsv'], - 'text/parquet': ['.parquet'], - 'text/json': ['.json'], + 'application/vnd.apache.parquet': ['.parquet'], + 'application/json': ['.json'], };🤖 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/components/useLocalFileLoader.ts` around lines 6 - 11, Update the LOCAL_DATA_ACCEPTED_FORMATS map to use standard MIME types: replace the 'text/parquet' key with 'application/vnd.apache.parquet' (or 'application/octet-stream' if you prefer a generic fallback) and replace 'text/json' with 'application/json', keeping the associated extension arrays intact; leave 'text/csv' and 'text/tsv' as-is and ensure the symbol LOCAL_DATA_ACCEPTED_FORMATS is updated accordingly wherever referenced.
🤖 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/components/useLocalFileLoader.ts`:
- Around line 22-34: The loop in useLocalFileLoader that uses
convertToValidColumnOrTableName and connector.loadFile can silently overwrite
tables when multiple files normalize to the same table name and also exposes raw
error objects in toast.error; fix by tracking already-used names (use the
existing createdTableNames array) and, before calling connector.loadFile, check
for collisions and if a name exists append a numeric suffix (e.g.,
`${baseName}_${n}`) until unique, then push the final name into
createdTableNames; also sanitize the toast error message by using error
instanceof Error ? error.message : String(error) (use this sanitized string in
toast.error description) and update toast.success to show the final table name
variable you actually created.
- Line 36: The call to refreshTableSchemas() can throw and currently isn't
handled; wrap the await refreshTableSchemas() invocation inside a try-catch in
useLocalFileLoader.ts, and in the catch block call the project's error-toast
utility to show a user-facing error message (e.g., "Failed to refresh table
schemas") and log the error; ensure the catch prevents misleading success
feedback (move or gate any existing success toasts for loaded files so they
don't display when schema refresh fails).
In `@apps/sqlrooms-cli-ui/src/workspace/sidebar/CliArtifactsSidebarSection.tsx`:
- Around line 43-66: The expanded sidebar currently maps artifactTabs.tabs and
renders nothing when the array is empty; add an explicit empty state so expanded
mode shows "No artifacts" like collapsed mode. In
CliArtifactsSidebarSection.tsx, before mapping artifactTabs.tabs (rendering
inside SidebarMenu), check if artifactTabs.tabs.length === 0 and return a
SidebarMenuItem (and SidebarMenuButton if you want consistent styling) that
displays the text "No artifacts" with the same sizing/truncation classes used
for items; otherwise proceed to the existing artifactTabs.tabs.map rendering.
Ensure you do not change selection logic (artifactTabs.selectedTabId) and keep
the same classNames for visual consistency.
---
Nitpick comments:
In `@apps/sqlrooms-cli-ui/src/components/useLocalFileLoader.ts`:
- Around line 6-11: Update the LOCAL_DATA_ACCEPTED_FORMATS map to use standard
MIME types: replace the 'text/parquet' key with 'application/vnd.apache.parquet'
(or 'application/octet-stream' if you prefer a generic fallback) and replace
'text/json' with 'application/json', keeping the associated extension arrays
intact; leave 'text/csv' and 'text/tsv' as-is and ensure the symbol
LOCAL_DATA_ACCEPTED_FORMATS is updated accordingly wherever referenced.
🪄 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: f26cb0a7-5340-4329-8c21-417db1bec8a6
⛔ Files ignored due to path filters (3)
apps/sqlrooms-cli-ui/public/logo.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpython/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
apps/sqlrooms-cli-ui/README.mdapps/sqlrooms-cli-ui/package.jsonapps/sqlrooms-cli-ui/src/artifactTypeIds.tsapps/sqlrooms-cli-ui/src/artifactTypes.tsxapps/sqlrooms-cli-ui/src/components/AssistantChatContainer.tsxapps/sqlrooms-cli-ui/src/components/DataSourcesPanel.tsxapps/sqlrooms-cli-ui/src/components/assistantUtils.tsapps/sqlrooms-cli-ui/src/components/useAssistantContextItems.tsapps/sqlrooms-cli-ui/src/components/useLocalFileLoader.tsapps/sqlrooms-cli-ui/src/context/formatRunContextInstructions.tsapps/sqlrooms-cli-ui/src/context/getRunContext.tsapps/sqlrooms-cli-ui/src/layout.tsxapps/sqlrooms-cli-ui/src/room.tsxapps/sqlrooms-cli-ui/src/store-types.tsapps/sqlrooms-cli-ui/src/store.tsapps/sqlrooms-cli-ui/src/style.cssapps/sqlrooms-cli-ui/src/workspace/ArtifactsContainerPanel.tsxapps/sqlrooms-cli-ui/src/workspace/CliWorkspaceTopbar.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliArtifactsSidebarSection.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliAssistantToggleButton.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliDataSidebarSection.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliSidebarBrand.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliSidebarFooterControls.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliSidebarToggleButton.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliWorkspaceSidebar.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/DbConnectionsSection.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/index.tsapps/sqlrooms-cli-ui/src/workspace/sidebar/useCliArtifactSidebarTabs.tsdev-docs/tasks/analysis-document-artifacts.mdpackages/ai-core/README.mdpackages/ai-core/__tests__/AiSlice.modelSelection.test.tspackages/ai-core/__tests__/ChatHistoryView.test.tsxpackages/ai-core/src/components/ChatHeader.tsxpackages/ai-core/src/components/ChatHistoryItem.tsxpackages/ai-core/src/components/ChatHistoryView.tsxpackages/ai-core/src/components/SessionActionsMenu.tsxpackages/ai-core/src/index.tspackages/ai/README.mdpackages/documents/README.mdpackages/documents/__tests__/BlockDocumentsSlice.test.tspackages/documents/src/BlockDocumentEditor/BlockDocumentEditorRoot.tsxpackages/documents/src/BlockDocumentEditor/extensions/BlockDocumentRichTextNode.tsxpackages/documents/src/BlockDocumentEditor/node-views/BlockDocumentRichTextNodeView.tsxpackages/documents/src/BlockDocumentSliceConfig.tspackages/documents/src/index.tspackages/schema-tree/src/nodes/ColumnTreeNode.tsxpackages/schema-tree/src/nodes/DatabaseTreeNode.tsxpackages/schema-tree/src/nodes/SchemaTreeNode.tsxpackages/schema-tree/src/nodes/TableTreeNode.tsxpackages/sql-editor/src/SqlEditorModal.tsxpackages/ui/src/components/tree.tsx
💤 Files with no reviewable changes (8)
- packages/documents/src/BlockDocumentEditor/node-views/BlockDocumentRichTextNodeView.tsx
- packages/documents/src/BlockDocumentEditor/extensions/BlockDocumentRichTextNode.tsx
- packages/documents/src/index.ts
- packages/documents/src/BlockDocumentEditor/BlockDocumentEditorRoot.tsx
- apps/sqlrooms-cli-ui/src/components/DataSourcesPanel.tsx
- apps/sqlrooms-cli-ui/package.json
- packages/documents/tests/BlockDocumentsSlice.test.ts
- packages/documents/src/BlockDocumentSliceConfig.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbdd085bbc
ℹ️ 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".
| useTools: false, | ||
| ...promptOptions, | ||
| }); | ||
| const title = cleanGeneratedSessionTitle(generatedTitle, maxTitleLength); |
There was a problem hiding this comment.
When the title-generation model call fails with the default AiSlice.sendPrompt, it resolves to the string error: can not generate response instead of throwing, and this path cleans that sentinel as if it were a valid title before renameSession runs. In no-API-key or transient provider-error scenarios, the first user message renames the chat to the error text and onError is never invoked; treat error results as failures before accepting the generated title.
Useful? React with 👍 / 👎.
| artifactsConfig.artifactOrder.filter((artifactId) => { | ||
| const artifact = artifactsConfig.artifactsById[artifactId]; | ||
| return ( | ||
| artifact && | ||
| isManagedType(managedTypes, artifact.type) && | ||
| isArtifactVisibleInTabs(artifact, {includeEmbedded}) | ||
| ); | ||
| return artifact && isManagedType(managedTypes, artifact.type); | ||
| }), |
There was a problem hiding this comment.
For workspaces saved before this change, embedded child artifacts still exist in artifactOrder with visibility: 'embedded'; ArtifactsSliceConfig.parse now strips that field, and this filter includes every managed artifact type, so those formerly hidden dashboards/pivots/documents become regular workspace tabs/sidebar entries after hydration. Please migrate/drop legacy embedded artifact records or preserve a legacy visibility check before surfacing them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/ai-core/src/hooks/useGenerateSessionTitle.ts (1)
176-182: ⚡ Quick winAvoid calling methods inside selectors to prevent unstable references.
Per coding guidelines, selectors should select stable raw state first. Calling
getCurrentSession()inside the selector returns a derived object that may trigger unnecessary re-renders. Consider selecting the raw session data:- const currentSessionId = useStoreWithAi((s) => s.ai.config.currentSessionId); - const currentSession = useStoreWithAi((s) => s.ai.getCurrentSession()); - const uiMessagesLength = useStoreWithAi( - (s) => s.ai.getCurrentSession()?.uiMessages?.length ?? 0, - ); + const currentSessionId = useStoreWithAi((s) => s.ai.config.currentSessionId); + const sessions = useStoreWithAi((s) => s.ai.config.sessions); + const currentSession = useMemo( + () => sessions.find((s) => s.id === currentSessionId), + [sessions, currentSessionId], + ); + const uiMessagesLength = currentSession?.uiMessages?.length ?? 0;This also consolidates three subscriptions into two and avoids redundant
getCurrentSession()calls.🤖 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 `@packages/ai-core/src/hooks/useGenerateSessionTitle.ts` around lines 176 - 182, The selectors in useGenerateSessionTitle are calling the getCurrentSession() method inside selectors (in lines 177 and 178-180), which violates best practices by creating unstable references that trigger unnecessary re-renders. Replace the getCurrentSession() method calls with selectors that access the raw session data directly from the store state (such as selecting from a sessions collection using the currentSessionId). This will eliminate the redundant method calls and consolidate the multiple useStoreWithAi subscriptions into fewer, more stable ones.Source: Coding guidelines
packages/artifacts/src/ai/artifactAiSlice.ts (1)
166-171: 💤 Low valueConsider more selective store subscription for autoSync.
The subscription on line 168 triggers
syncCurrentArtifactAiSessionon every state change. While the early-return guards (lines 134, 148-155) and cleanup short-circuit (lines 120-125) prevent wasteful updates, a more targeted subscription that listens only toartifacts.config.currentArtifactIdandai.configchanges would reduce unnecessary invocations.🤖 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 `@packages/artifacts/src/ai/artifactAiSlice.ts` around lines 166 - 171, The subscription in the initialize function currently triggers syncCurrentArtifactAiSession on every state change, which is inefficient. Modify the store subscription to use a selector-based approach that only triggers the sync function when specific state slices like artifacts.config.currentArtifactId and ai.config actually change. This can be done by passing a selector function to the store subscribe method (or using a selector hook if available) so that syncCurrentArtifactAiSession is called only when the relevant parts of the state change, not on every single state update.
🤖 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 `@packages/ai-core/src/hooks/useGenerateSessionTitle.ts`:
- Around line 176-182: The selectors in useGenerateSessionTitle are calling the
getCurrentSession() method inside selectors (in lines 177 and 178-180), which
violates best practices by creating unstable references that trigger unnecessary
re-renders. Replace the getCurrentSession() method calls with selectors that
access the raw session data directly from the store state (such as selecting
from a sessions collection using the currentSessionId). This will eliminate the
redundant method calls and consolidate the multiple useStoreWithAi subscriptions
into fewer, more stable ones.
In `@packages/artifacts/src/ai/artifactAiSlice.ts`:
- Around line 166-171: The subscription in the initialize function currently
triggers syncCurrentArtifactAiSession on every state change, which is
inefficient. Modify the store subscription to use a selector-based approach that
only triggers the sync function when specific state slices like
artifacts.config.currentArtifactId and ai.config actually change. This can be
done by passing a selector function to the store subscribe method (or using a
selector hook if available) so that syncCurrentArtifactAiSession is called only
when the relevant parts of the state change, not on every single state update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aaeef111-2cc8-4771-bff3-c2215308a8cf
📒 Files selected for processing (41)
apps/sqlrooms-cli-ui/src/components/AssistantChatContainer.tsxapps/sqlrooms-cli-ui/src/components/useGenSessionTitle.tsapps/sqlrooms-cli-ui/src/components/useLocalFileLoader.tsapps/sqlrooms-cli-ui/src/context/getRunContext.tsapps/sqlrooms-cli-ui/src/store-types.tsapps/sqlrooms-cli-ui/src/store.tsapps/sqlrooms-cli-ui/src/workspace/ArtifactsContainerPanel.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliArtifactsSidebarSection.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/CliAssistantToggleButton.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/useCliArtifactSidebarTabs.tsexamples/canvas/src/store.tsexamples/kepler/src/store.tsexamples/notebook/src/store.tsexamples/pivot/src/store.tspackages/ai-config/src/AiSliceConfig.tspackages/ai-core/README.mdpackages/ai-core/src/AiSlice.tspackages/ai-core/src/components/ChatHeader.tsxpackages/ai-core/src/components/ChatHistoryView.tsxpackages/ai-core/src/hooks/useGenerateSessionTitle.tspackages/ai-core/src/index.tspackages/ai/README.mdpackages/ai/src/index.tspackages/artifacts/README.mdpackages/artifacts/__tests__/ArtifactContextAiTools.test.tspackages/artifacts/__tests__/ArtifactsSlice.test.tspackages/artifacts/__tests__/artifactAiSessions.test.tspackages/artifacts/package.jsonpackages/artifacts/src/ArtifactsSlice.tspackages/artifacts/src/ArtifactsSliceConfig.tspackages/artifacts/src/ai/artifactAiSessionHelpers.tspackages/artifacts/src/ai/artifactAiSlice.tspackages/artifacts/src/ai/artifactContextTools.tspackages/artifacts/src/ai/index.tspackages/artifacts/src/artifactTabs.tsxpackages/artifacts/src/index.tspackages/documents/__tests__/BlockDocumentCommands.test.tspackages/documents/__tests__/documentCommands.test.tspackages/documents/__tests__/documentsCrdt.test.tspackages/documents/src/BlockDocumentCommands.tspackages/documents/src/crdt.ts
💤 Files with no reviewable changes (3)
- packages/artifacts/src/ArtifactsSliceConfig.ts
- packages/documents/src/BlockDocumentCommands.ts
- packages/artifacts/src/index.ts
✅ Files skipped from review due to trivial changes (5)
- packages/ai-config/src/AiSliceConfig.ts
- examples/notebook/src/store.ts
- packages/ai/README.md
- packages/ai-core/README.md
- packages/artifacts/README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/sqlrooms-cli-ui/src/workspace/sidebar/CliAssistantToggleButton.tsx
- apps/sqlrooms-cli-ui/src/workspace/sidebar/useCliArtifactSidebarTabs.ts
- apps/sqlrooms-cli-ui/src/components/AssistantChatContainer.tsx
- apps/sqlrooms-cli-ui/src/workspace/sidebar/CliArtifactsSidebarSection.tsx
- packages/ai-core/src/components/ChatHistoryView.tsx
- apps/sqlrooms-cli-ui/src/components/useLocalFileLoader.ts
- packages/ai-core/src/components/ChatHeader.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bd1bc8cf5
ℹ️ 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".
| return {status: 'unchanged', title}; | ||
| } | ||
|
|
||
| renameSession(session.id, title); |
There was a problem hiding this comment.
If the user manually edits an Untitled chat title while the async title-generation request is in flight, this call still renames the session using the stale session object captured before the request. In the CLI the title is now directly editable, and model calls can take long enough for this race to overwrite the user's custom title; re-read the latest session name or otherwise confirm it is still a default title immediately before calling renameSession.
Useful? React with 👍 / 👎.
Summary
This PR refreshes the SQLRooms CLI workspace around artifact-first navigation and artifact-scoped AI assistance.
Validation
pnpm --filter @sqlrooms/artifacts testpnpm --filter @sqlrooms/documents testpnpm --filter @sqlrooms/artifacts typecheckpnpm --filter @sqlrooms/documents typecheckpnpm --filter @sqlrooms/ai-core testpnpm --filter @sqlrooms/ai-core typecheckpnpm --filter sqlrooms-cli-app typecheck