Repository navigation
fix(cli): navigate artifact on fork source links - #786
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds artifact-scoped AI session switching, chart-block copying during artifact chat handoff, worksheet mutation retry logic in the worksheet agent tool, and fork-source chat UI wiring, with tests covering the new behaviors. ChangesArtifact navigation, handoff, worksheet agent, and chat UI
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant switchToArtifactAiSession
participant RoomState
Caller->>switchToArtifactAiSession: sessionId
switchToArtifactAiSession->>RoomState: check ai.config.sessions[sessionId]
alt session exists
switchToArtifactAiSession->>RoomState: setCurrentArtifact(ownerArtifactId)
switchToArtifactAiSession->>RoomState: ai.switchSession(sessionId)
switchToArtifactAiSession-->>Caller: true
else session missing
switchToArtifactAiSession-->>Caller: false
end
sequenceDiagram
participant commandMiddleware
participant SourceArtifact
participant onChatFinish
participant TargetArtifact
commandMiddleware->>SourceArtifact: load source session and last user message
commandMiddleware->>commandMiddleware: compute shouldCopySourceChartBlocks
commandMiddleware->>SourceArtifact: setSessionArtifact, clear draft context, set current artifact
onChatFinish->>onChatFinish: allow source or target current artifact
alt shouldCopySourceChartBlocks
onChatFinish->>SourceArtifact: getBlocks(chart)
onChatFinish->>TargetArtifact: append cloned chart blocks
end
onChatFinish->>TargetArtifact: reset draft context, set current artifact, select latest session
sequenceDiagram
participant execute
participant BlockDocument
participant runWorksheetSubAgent
execute->>BlockDocument: ensureBlockDocument, getBlocks(initial count)
execute->>runWorksheetSubAgent: attempt 1
runWorksheetSubAgent-->>execute: result + tool calls
execute->>BlockDocument: getBlocks(recheck count)
execute->>execute: hasWorksheetMutationToolCall?
alt no mutation detected
execute->>runWorksheetSubAgent: attempt 2 with augmented prompt
runWorksheetSubAgent-->>execute: result + tool calls
execute->>execute: recheck mutation
end
sequenceDiagram
participant User
participant ChatMessagesContainer
participant AssistantChatContainer
participant switchToArtifactAiSession
User->>ChatMessagesContainer: click fork provenance
ChatMessagesContainer->>AssistantChatContainer: onSwitchToForkSource(sourceSession, forkOrigin)
AssistantChatContainer->>switchToArtifactAiSession: switch to source session
switchToArtifactAiSession-->>AssistantChatContainer: current artifact and session updated
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
2b92470 to
b58b8fc
Compare
b58b8fc to
cca4566
Compare
![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: b58b8fc13f
ℹ️ 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 ( | ||
| currentBlockCount === initialBlockCount && | ||
| !hasWorksheetMutationToolCall(result.agentToolCalls) |
There was a problem hiding this comment.
This new no-mutation check also applies to read-only worksheet requests. The CLI system prompt tells the model to use block_document_agent when the primary artifact is a worksheet, including when the user asks to inspect it, so a valid request like “what columns are in this worksheet?” can legitimately only call query/list/schema tools and return text. With this condition, that successful answer is retried and then converted into an error unless a block is mutated, which breaks read-only worksheet Q&A and can pressure the agent to add unwanted content.
Useful? React with 👍 / 👎.
| if (handoff.shouldCopySourceChartBlocks) { | ||
| copySourceChartBlocksToEmptyTarget( |
There was a problem hiding this comment.
This branch can run for any handoff target in CLI_ARTIFACT_TYPES, which includes non-block-document artifacts such as dashboards, SQL queries, HTML apps, and Python artifacts. For a prompt like “copy the chart to a new dashboard”, blockDocuments.getBlocks(targetArtifact.id) returns empty and appendBlocks creates orphan block-document content under the dashboard id instead of updating the dashboard, polluting workspace state with invisible content. Please guard this copy path to worksheet/block-document targets before appending blocks.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/sqlrooms-cli-ui/src/ai/createWorksheetAgentTool.ts (1)
427-467: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMetadata only reflects the final attempt's tool calls.
When a retry occurs,
resultis overwritten by the secondrunWorksheetSubAgentcall, socalculateAgentResultMetadata(line 464-467) only sees the second run'sagentToolCalls. Any tool calls (e.g. exploratory queries) from the discarded first attempt are silently dropped from usage/cost metadata, undercounting actual work done for that request.💡 Proposed fix to accumulate tool calls across retries
- let result: AgentRunResult = await runWorksheetSubAgent(intent); + let result: AgentRunResult = await runWorksheetSubAgent(intent); + let allToolCalls = result.agentToolCalls ?? []; let currentBlockCount = blockDocumentAdapter.getBlocks(worksheetId)?.length ?? 0; if ( currentBlockCount === initialBlockCount && !hasWorksheetMutationToolCall(result.agentToolCalls) ) { result = await runWorksheetSubAgent(`${intent} ...`); + allToolCalls = allToolCalls.concat(result.agentToolCalls ?? []); currentBlockCount = blockDocumentAdapter.getBlocks(worksheetId)?.length ?? 0; } ... - const metadata = calculateAgentResultMetadata( - undefined, - result.agentToolCalls, - ); + const metadata = calculateAgentResultMetadata(undefined, allToolCalls);🤖 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/ai/createWorksheetAgentTool.ts` around lines 427 - 467, The metadata calculation in createWorksheetAgentTool is only using the final retry’s tool calls because runWorksheetSubAgent overwrites result on the second attempt. Preserve and merge agentToolCalls from both attempts before calling calculateAgentResultMetadata, so exploratory calls from the first run are included in the final usage/cost metadata. Keep the retry logic intact in runWorksheetSubAgent, but aggregate the first and second AgentRunResult.tool calls instead of discarding the initial result.
🧹 Nitpick comments (2)
apps/sqlrooms-cli-ui/src/__tests__/artifactAiNavigation.test.ts (1)
6-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for sessions without an owning artifact.
All three tests set
sessionArtifactIdto a truthy value; none cover the case wheregetSessionArtifactIdreturnsundefined(e.g., a legacy/global session), which skipssetCurrentArtifactentirely. Consider adding a case for that branch.🤖 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/__tests__/artifactAiNavigation.test.ts` around lines 6 - 47, Add a test case in artifactAiNavigation.test.ts that covers the branch where artifactAi.getSessionArtifactId returns undefined, since createMockStore currently only exercises truthy sessionArtifactId values. Update or extend the existing createMockStore helper and related artifactAi navigation test(s) so the session-selection flow is verified for a legacy/global session and confirms setCurrentArtifact is skipped while switchSession still behaves as expected.apps/sqlrooms-cli-ui/src/__tests__/artifactChatHandoff.test.ts (1)
171-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a non-chart-copy prompt with an empty target.
Both tests use a "same chart" prompt. Add a case with a prompt that doesn't match
isSameChartRequest(empty target) to confirmappendBlocksis skipped due toshouldCopySourceChartBlocksbeingfalse, not just because the target already has blocks.🤖 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/__tests__/artifactChatHandoff.test.ts` around lines 171 - 229, The current tests only cover same-chart handoff behavior, so add a case in createArtifactChatHandoffController that uses a prompt not matched by isSameChartRequest while the target artifact is still empty. Reuse triggerHandoff and createState, then assert appendBlocks is not called because shouldCopySourceChartBlocks is false, confirming the skip happens for a non-chart-copy prompt rather than only when targetBlocks already exist.
🤖 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/ai/__tests__/createWorksheetAgentTool.test.ts`:
- Around line 10-18: The test mock for BlockDocumentAiAdapter.addBlock is
letting the block parameter default to unknown, which breaks type-checking when
accessing block.id. Update the addBlock jest.fn in
createWorksheetAgentTool.test.ts to explicitly type its parameters using the
addBlock signature from BlockDocumentAiAdapter so block is no longer unknown,
and keep the mock behavior of pushing the block and returning block.id.
In `@packages/ai-core/src/components/ChatMessagesContainer.tsx`:
- Around line 14-18: Add TSDoc to the exported ChatForkSourceSwitchHandler type
in ChatMessagesContainer.tsx so this new public API in `@sqlrooms/ai-core` is
documented per the package guidelines. Place a concise description above the
type alias explaining what the handler is used for, and mention the parameters
sourceSession and forkOrigin so the exported surface is properly documented
alongside the other public types.
---
Outside diff comments:
In `@apps/sqlrooms-cli-ui/src/ai/createWorksheetAgentTool.ts`:
- Around line 427-467: The metadata calculation in createWorksheetAgentTool is
only using the final retry’s tool calls because runWorksheetSubAgent overwrites
result on the second attempt. Preserve and merge agentToolCalls from both
attempts before calling calculateAgentResultMetadata, so exploratory calls from
the first run are included in the final usage/cost metadata. Keep the retry
logic intact in runWorksheetSubAgent, but aggregate the first and second
AgentRunResult.tool calls instead of discarding the initial result.
---
Nitpick comments:
In `@apps/sqlrooms-cli-ui/src/__tests__/artifactAiNavigation.test.ts`:
- Around line 6-47: Add a test case in artifactAiNavigation.test.ts that covers
the branch where artifactAi.getSessionArtifactId returns undefined, since
createMockStore currently only exercises truthy sessionArtifactId values. Update
or extend the existing createMockStore helper and related artifactAi navigation
test(s) so the session-selection flow is verified for a legacy/global session
and confirms setCurrentArtifact is skipped while switchSession still behaves as
expected.
In `@apps/sqlrooms-cli-ui/src/__tests__/artifactChatHandoff.test.ts`:
- Around line 171-229: The current tests only cover same-chart handoff behavior,
so add a case in createArtifactChatHandoffController that uses a prompt not
matched by isSameChartRequest while the target artifact is still empty. Reuse
triggerHandoff and createState, then assert appendBlocks is not called because
shouldCopySourceChartBlocks is false, confirming the skip happens for a
non-chart-copy prompt rather than only when targetBlocks already exist.
🪄 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: a1d0ebbb-5f75-420e-bda7-d8d4b4978617
📒 Files selected for processing (8)
apps/sqlrooms-cli-ui/src/__tests__/artifactAiNavigation.test.tsapps/sqlrooms-cli-ui/src/__tests__/artifactChatHandoff.test.tsapps/sqlrooms-cli-ui/src/ai/__tests__/createWorksheetAgentTool.test.tsapps/sqlrooms-cli-ui/src/ai/createWorksheetAgentTool.tsapps/sqlrooms-cli-ui/src/artifactAiNavigation.tsapps/sqlrooms-cli-ui/src/artifactChatHandoff.tsapps/sqlrooms-cli-ui/src/components/AssistantChatContainer.tsxpackages/ai-core/src/components/ChatMessagesContainer.tsx
cca4566 to
25a307a
Compare
25a307a to
f7051fe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7051fe53d
ℹ️ 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".
| /\b(add|append|build|copy|create|delete|duplicate|edit|embed|graph|insert|make|modify|move|plot|put|remove|replace|update|visuali[sz]e)\b/.test( | ||
| normalized, | ||
| ) || | ||
| /\b(chart|dashboard|data table explorer|html app|map|text block|worksheet block)\b/.test( |
There was a problem hiding this comment.
The new heuristic still treats any intent containing chart/dashboard as requiring a worksheet mutation unless it starts with the small allow-list above. For read-only worksheet prompts such as “show me the chart in this worksheet” or “summarize this dashboard block”, the sub-agent can correctly answer by listing/querying blocks without changing the document, but this check retries it and then returns a no-mutation error. Fresh evidence is this keyword branch, which still catches read-only chart/dashboard inspection phrasing that the added what columns... test does not cover.
Useful? React with 👍 / 👎.
Summary
Fixes CLI artifact chat handoff navigation after an AI-created worksheet/block document fork.
Root Cause
The fork provenance link only switched AI sessions. In the CLI app, artifact AI sync is driven by the current artifact. When the current artifact was still the fork target, switching to the source chat alone was immediately overwritten by sync, so the link appeared to do nothing and subsequent prompts could continue targeting the forked worksheet.
Validation
pnpm --filter sqlrooms-cli-app test -- --runTestsByPath src/__tests__/artifactAiNavigation.test.tspnpm --filter @sqlrooms/ai-core typecheckpnpm --filter sqlrooms-cli-app typecheckpnpm exec prettier --check packages/ai-core/src/components/ChatMessagesContainer.tsx apps/sqlrooms-cli-ui/src/components/AssistantChatContainer.tsx apps/sqlrooms-cli-ui/src/artifactAiNavigation.ts apps/sqlrooms-cli-ui/src/__tests__/artifactAiNavigation.test.tspnpm buildknip,turbo typecheck, circular dependency check,turbo testSummary by CodeRabbit