Repository navigation
refactor: Refactor CLI artifact content host - #741
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:
📝 WalkthroughWalkthroughA new ChangesCore artifact workspace abstraction
useArtifactTabs refactor to use workspace
CLI UI panel and sidebar refactor
Documentation
Sequence DiagramsequenceDiagram
participant ArtifactsContainerPanel as Panel
participant CliArtifactWorkspaceActionsContext as Context
participant CliArtifactContentHost as ContentHost
participant SelectedCliArtifactContent as ArtifactContent
participant useArtifactWorkspace as Workspace
Panel->>Workspace: initialize workspace (types, selectFallback)
Workspace->>Workspace: read currentArtifactId from store
Workspace->>Workspace: derive selectedArtifactId with fallback
Panel->>Context: provide selectedArtifactId, artifactIds, selectArtifact
Context->>ContentHost: context available
alt artifact selected
ContentHost->>Workspace: read selectedArtifact from context
ContentHost->>ContentHost: trigger ensureArtifact effect
ContentHost->>ArtifactContent: render with selected artifact
ArtifactContent->>ArtifactContent: render type-specific component
else content prop provided (chooser open)
ContentHost->>ContentHost: render content
else no selection
ContentHost->>ContentHost: render emptyContent
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
ilyabo
changed the title
![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: dc3d213a74
ℹ️ 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 useMemo( | ||
| () => ({ | ||
| selectedArtifactId: currentArtifactId, |
There was a problem hiding this comment.
When a rehydrated or synced workspace has artifacts but no valid currentArtifactId (the artifacts schema permits it, and the CRDT mirror clears invalid current selections), this exposes selectedArtifactId as undefined, so CliArtifactContentHost shows the new-artifact start screen instead of opening an existing artifact. The removed ArtifactTabs host repaired this by adding workspace tabs and syncing currentArtifactId from the active tab, so existing workspaces in that state now land on the create screen until the user manually picks/closes. Please initialize/fall back to an artifact from artifactIds when the current ID is missing.
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
🧹 Nitpick comments (2)
apps/sqlrooms-cli-ui/src/workspace/sidebar/useCliArtifactSidebarTabs.ts (2)
6-91: ⚡ Quick winAdd TSDoc to the exported hook.
As per coding guidelines, exported functions in TypeScript files should have TSDoc comments. Adding documentation would help clarify the hook's purpose and the new store-driven selection model, especially following this refactor.
📝 Suggested TSDoc
+/** + * Hook providing sidebar tab state and actions for CLI artifacts. + * + * Selection is driven by the artifact store's currentArtifactId rather than layout tabs. + * Returns artifact tabs with running AI session counts, and callbacks for selection, + * rename, and deletion operations. + */ export function useCliArtifactSidebarTabs() {🤖 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/workspace/sidebar/useCliArtifactSidebarTabs.ts` around lines 6 - 91, The exported hook function useCliArtifactSidebarTabs is missing TSDoc documentation. Add a TSDoc comment block above the function definition that describes the hook's purpose, what it returns (including the structure of the return object with properties like artifactTypes, deleteArtifact, renameArtifact, selectedTabId, selectArtifact, and tabs), and any relevant details about how it manages artifact state and the store-driven selection model.Source: Coding guidelines
13-15: 💤 Low valueConsider renaming
selectedTabIdtoselectedArtifactIdfor clarity.Now that the refactor has moved away from the tab-based model, the variable name
selectedTabIdis misleading. The upstreamCliArtifactWorkspaceActionsContextusesselectedArtifactIdfor the same concept.Renaming would improve consistency and make the code more self-documenting.
🤖 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/workspace/sidebar/useCliArtifactSidebarTabs.ts` around lines 13 - 15, Rename the variable `selectedTabId` to `selectedArtifactId` in the useCliArtifactSidebarTabs hook. This variable holds the value from state.artifacts.config.currentArtifactId and should be renamed throughout the file to match the naming convention used in CliArtifactWorkspaceActionsContext and reflect the artifact-based model rather than a tab-based one. Update all references to selectedTabId with selectedArtifactId to maintain consistency across the component.
🤖 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/workspace/sidebar/useCliArtifactSidebarTabs.ts`:
- Around line 61-66: The selectArtifact callback in useCliArtifactSidebarTabs.ts
is missing validation to verify that the artifact exists in the artifactsById
map before calling setCurrentArtifact. Add a defensive check at the beginning of
the selectArtifact function that verifies the artifactId exists in artifactsById
and only proceeds with setCurrentArtifact and setShowArtifactChooser if the
validation passes. This will make the hook more defensive and consistent with
the upstream selectArtifact implementation in
useCliArtifactWorkspaceActionState.
---
Nitpick comments:
In `@apps/sqlrooms-cli-ui/src/workspace/sidebar/useCliArtifactSidebarTabs.ts`:
- Around line 6-91: The exported hook function useCliArtifactSidebarTabs is
missing TSDoc documentation. Add a TSDoc comment block above the function
definition that describes the hook's purpose, what it returns (including the
structure of the return object with properties like artifactTypes,
deleteArtifact, renameArtifact, selectedTabId, selectArtifact, and tabs), and
any relevant details about how it manages artifact state and the store-driven
selection model.
- Around line 13-15: Rename the variable `selectedTabId` to `selectedArtifactId`
in the useCliArtifactSidebarTabs hook. This variable holds the value from
state.artifacts.config.currentArtifactId and should be renamed throughout the
file to match the naming convention used in CliArtifactWorkspaceActionsContext
and reflect the artifact-based model rather than a tab-based one. Update all
references to selectedTabId with selectedArtifactId to maintain consistency
across the component.
🪄 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: 875672b8-0af0-4e30-9706-518f8b308dca
📒 Files selected for processing (2)
apps/sqlrooms-cli-ui/src/workspace/ArtifactsContainerPanel.tsxapps/sqlrooms-cli-ui/src/workspace/sidebar/useCliArtifactSidebarTabs.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0325d6ba05
ℹ️ 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".
| selectedArtifactId: | ||
| currentArtifactId && artifactsById[currentArtifactId] | ||
| ? currentArtifactId | ||
| : artifactIds[0], |
There was a problem hiding this comment.
When a persisted or synced workspace has artifacts but currentArtifactId is unset or stale, this fallback only exposes artifactIds[0] through the local context. Fresh evidence: this version added the fallback here, but it still never calls setCurrentArtifact, so the content pane can render an artifact while global-selection consumers such as CliWorkspaceTopbar title/delete controls and AssistantChatContainer current context still behave as if no artifact is selected until the user manually reselects it. Set the current artifact when taking this fallback.
Useful? React with 👍 / 👎.
| <Component | ||
| key={artifact.id} |
There was a problem hiding this comment.
This renders only one keyed artifact component, so selecting another artifact unmounts the previous panel. The removed ArtifactTabs passed forceMountContent, and TabsLayout.TabContent documents that mode as preserving local panel state/setup work; with this replacement, unsaved local state such as AppBuilderArtifact's prompt/template/name/status draft is lost if the user briefly switches artifacts before generating or saving. Keep prior panels mounted or hoist those drafts before replacing the force-mounted content path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47479d5ebc
ℹ️ 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 selectedArtifactId = | ||
| currentArtifactId && artifactsById[currentArtifactId] | ||
| ? currentArtifactId | ||
| : artifactIds[0]; |
There was a problem hiding this comment.
When a persisted or synced workspace has currentArtifactId pointing at an artifact whose type is not one of CLI_ARTIFACT_TYPES (the artifact schema allows arbitrary string types), this accepts it as the selected artifact merely because it exists in artifactsById. CliArtifactContentHost then calls ensureArtifact with that unregistered type, which the CLI artifacts slice rejects, instead of falling back to a valid CLI artifact from artifactIds; the workspace can crash or render no panel even though CLI artifacts are present. Please only use the current id here when it is included in the filtered CLI artifact set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c7ba9a4b0
ℹ️ 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".
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (!selectedArtifactId || selectedArtifactId === currentArtifactId) return; |
There was a problem hiding this comment.
When currentArtifactId is stale or points at a non-CLI artifact and artifactIds is empty, this early return leaves the invalid/non-CLI id in global state even though the content host renders the new-artifact screen. Fresh evidence here is that the new fallback only persists selectedArtifactId, so with no CLI fallback consumers such as the assistant context (which gates on a truthy currentArtifactId) or the topbar can still act on a hidden/unsupported artifact; the previous ArtifactTabs sync cleared the current artifact when no managed tab was active. Clear currentArtifactId in this branch instead of returning with the stale selection intact.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/artifacts/src/artifactTabs.tsx (1)
226-234: 💤 Low valueConsider removing redundant
useMemowrappers.These memos return the already-memoized values from
useArtifactWorkspacewith those same values as dependencies, providing no additional memoization benefit.♻️ Suggested simplification
- const artifactOrder = useMemo( - () => artifactWorkspace.artifactIds, - [artifactWorkspace.artifactIds], - ); - - const tabs = useMemo<ArtifactTabDescriptor[]>( - () => artifactWorkspace.artifacts, - [artifactWorkspace.artifacts], - ); + const artifactOrder = artifactWorkspace.artifactIds; + const tabs: ArtifactTabDescriptor[] = artifactWorkspace.artifacts;🤖 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/artifactTabs.tsx` around lines 226 - 234, The useMemo wrappers for both artifactOrder and tabs are redundant because they return the same values that are already memoized from useArtifactWorkspace with identical dependencies, providing no additional memoization benefit. Remove the useMemo hooks from both variables: replace the artifactOrder useMemo wrapper with a direct assignment to artifactWorkspace.artifactIds, and replace the tabs useMemo wrapper with a direct assignment to artifactWorkspace.artifacts, eliminating the unnecessary dependency arrays.
🤖 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 `@packages/artifacts/src/artifactWorkspace.tsx`:
- Around line 83-88: The typesKey calculation at line 83 fails to differentiate
between undefined (manage all types) and an empty array (manage none), since
both map to an empty string. This causes the managedTypes useMemo hook to not
recalculate when the caller switches between these two cases. Modify the
typesKey calculation to produce distinct values for each case, such as by
including a prefix marker (e.g., use a different sentinel value when
options.types is undefined versus when it's an empty array) so the dependency
array can properly detect the change.
---
Nitpick comments:
In `@packages/artifacts/src/artifactTabs.tsx`:
- Around line 226-234: The useMemo wrappers for both artifactOrder and tabs are
redundant because they return the same values that are already memoized from
useArtifactWorkspace with identical dependencies, providing no additional
memoization benefit. Remove the useMemo hooks from both variables: replace the
artifactOrder useMemo wrapper with a direct assignment to
artifactWorkspace.artifactIds, and replace the tabs useMemo wrapper with a
direct assignment to artifactWorkspace.artifacts, eliminating the unnecessary
dependency arrays.
🪄 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: ea7497e1-ace0-4d04-bd17-99baae659fbd
📒 Files selected for processing (5)
apps/sqlrooms-cli-ui/src/workspace/ArtifactsContainerPanel.tsxpackages/artifacts/README.mdpackages/artifacts/src/artifactTabs.tsxpackages/artifacts/src/artifactWorkspace.tsxpackages/artifacts/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/artifacts/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/sqlrooms-cli-ui/src/workspace/ArtifactsContainerPanel.tsx
| const typesKey = options.types?.join('\u0000') ?? ''; | ||
| const managedTypes = useMemo( | ||
| () => (options.types ? [...options.types] : undefined), | ||
| // Keep inline arrays like types={['notebook']} stable by value. | ||
| [typesKey], | ||
| ); |
There was a problem hiding this comment.
Differentiate undefined and empty types in memo key.
typesKey at Line 83 maps both undefined and [] to '', so managedTypes can remain stale when the caller switches between “manage all” and “manage none”.
Suggested fix
- const typesKey = options.types?.join('\u0000') ?? '';
+ const typesKey =
+ options.types === undefined ? 'all' : `managed:${options.types.join('\u0000')}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const typesKey = options.types?.join('\u0000') ?? ''; | |
| const managedTypes = useMemo( | |
| () => (options.types ? [...options.types] : undefined), | |
| // Keep inline arrays like types={['notebook']} stable by value. | |
| [typesKey], | |
| ); | |
| const typesKey = | |
| options.types === undefined ? 'all' : `managed:${options.types.join('\u0000')}`; | |
| const managedTypes = useMemo( | |
| () => (options.types ? [...options.types] : undefined), | |
| // Keep inline arrays like types={['notebook']} stable by value. | |
| [typesKey], | |
| ); |
🤖 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/artifactWorkspace.tsx` around lines 83 - 88, The
typesKey calculation at line 83 fails to differentiate between undefined (manage
all types) and an empty array (manage none), since both map to an empty string.
This causes the managedTypes useMemo hook to not recalculate when the caller
switches between these two cases. Modify the typesKey calculation to produce
distinct values for each case, such as by including a prefix marker (e.g., use a
different sentinel value when options.types is undefined versus when it's an
empty array) so the dependency array can properly detect the change.
Summary
ArtifactTabshost with a CLI-local selected-artifact content host.artifacts.config.currentArtifactIdinstead of opening hidden layout tabs.Notes
Inactive artifact React trees are no longer kept mounted by the CLI workspace host. Durable artifact state remains in the room/artifact-specific stores, but transient component-local state may reset when switching artifacts.
Validation
pnpm buildpnpm --filter sqlrooms-cli-app typecheckpnpm --filter sqlrooms-cli-app buildknip,turbo typecheck,turbo testManual browser smoke testing was limited because the in-app browser control failed to initialize in this session and the Vite dev server socket was not reachable from a separate shell, despite the production build succeeding.
Summary by CodeRabbit
useArtifactWorkspacehook for selecting and rendering the current artifact in non-tab surfaces.useArtifactWorkspace.