Repository navigation
refactor: Adopt chat terminology for AI sessions - #714
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRenames the AI session model from ChangesChat Session Model Migration
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 docstrings
🧪 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: 20fac629bd
ℹ️ 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 {analysisResults: _legacyAnalysisResults, ...session} = | ||
| sessionWithLegacy; |
There was a problem hiding this comment.
For rooms saved before this refactor where a failed or cancelled request is represented only in analysisResults.errorMessage, this drops the legacy result before that error is copied into uiMessages. After parsing, getAnalysisResultsFromUiMessages only reads message.metadata.sqlrooms.errorMessage, so those historical failures reload as prompt-only/incomplete turns with no error. Please transfer legacy errorMessage and completion state onto the synthesized or matching user message before removing analysisResults.
Useful? React with 👍 / 👎.
| return getAnalysisResultsFromUiMessages( | ||
| currentSession.uiMessages as UIMessage[], | ||
| {isRunning: currentSession.isRunning}, | ||
| ); |
There was a problem hiding this comment.
When existing consumers keep the previous pattern useStoreWithAi((s) => s.ai.getAnalysisResults()), this now returns a freshly mapped array on every store snapshot even when uiMessages has not changed. Zustand/useSyncExternalStore treats that as a changed snapshot and can trigger the cached-snapshot warning or render loop; the old implementation returned the stable stored analysisResults array. Please cache the derived results per session/messages reference or avoid exposing this getter for selector use.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ai-config/src/migration/AnalysisSession-v0.26.0.ts (1)
84-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve legacy
errorMessagewhen synthesizinguiMessages.This migration drops
analysisResults, but it never mapsresult.errorMessageonto the synthesized messages. Downstream,getChatRequestErrorMessage()inpackages/ai-core/src/chatTurns.tsonly readsmessage.metadata.sqlrooms.errorMessage, so failed legacy sessions rehydrate as turns with no error onceanalysisResultsis stripped.Suggested fix
if (needsMessageSynthesis) { for (const result of analysisResults) { if (!isObject(result)) continue; const id = (result.id as string) || ''; const prompt = (result.prompt as string) || ''; + const errorMessage = + isObject(result.errorMessage) && + typeof result.errorMessage.error === 'string' + ? {error: result.errorMessage.error} + : undefined; const streamMessage = (result.streamMessage as UnknownRecord) || {}; const parts = (streamMessage.parts as UnknownRecord[]) || []; // Create user message for the prompt if (prompt) { synthesizedMessages.push({ id, role: 'user', + ...(errorMessage + ? {metadata: {sqlrooms: {errorMessage}}} + : {}), parts: [{type: 'text', text: prompt}], }); }🤖 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-config/src/migration/AnalysisSession-v0.26.0.ts` around lines 84 - 152, The code synthesizing messages from analysisResults does not preserve the errorMessage field from the result object. This causes downstream code in getChatRequestErrorMessage() to lose error information when reading message.metadata.sqlrooms.errorMessage. Extract the errorMessage from each result in the loop (similar to how prompt and streamMessage are extracted), and when creating synthesized messages, add a metadata object with the structure metadata.sqlrooms.errorMessage to preserve the error information for downstream processing.packages/ai-core/src/AiSlice.ts (1)
281-297:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the same session normalization in
setConfig().This new cleanup only runs during slice construction.
ai.setConfig()still stores incoming sessions verbatim, so any caller restoring/importing config through the public setter can reintroduceisRunning: truesessions and incompleteuiMessagesthat this block explicitly fixes on startup. Please extract this into a shared normalizer and use it from both ingress paths.🤖 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/AiSlice.ts` around lines 281 - 297, The session cleanup logic (running cleanupPendingUiMessages, fixIncompleteToolCalls, and resetting isRunning to false) is only applied during slice construction but not when setConfig() is called. This means callers can bypass normalization by restoring config through the public setter, reintroducing problematic state like isRunning: true or incomplete tool calls. Extract the session normalization logic from the slice constructor (the mapping over params.config.sessions that calls cleanupPendingUiMessages and fixIncompleteToolCalls) into a shared helper function or method, then apply this normalizer to the sessions parameter in both the constructor code shown and the setConfig() method to ensure consistent normalization regardless of how config is ingested.
🤖 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 `@docs/upgrade-guide.md`:
- Around line 304-308: The code example showing Chat.Messages is incomplete and
misleading because it omits the required Chat.Root wrapper that provides the
chat/session context. Update the example snippet to wrap Chat.Messages with its
parent Chat.Root component, ensuring the example is self-contained and
immediately copy-pasteable for users following the upgrade guide.
In `@packages/ai-core/__tests__/migration.test.ts`:
- Around line 31-33: The test assertions at
packages/ai-core/__tests__/migration.test.ts lines 31-33 and lines 51-53 use
toBeUndefined() to verify that analysisResults has been removed, but this
approach does not fully validate the migration contract since toBeUndefined()
passes even if the property exists with an explicit undefined value. Replace
both instances of toBeUndefined() assertions with
not.toHaveProperty('analysisResults') or an equivalent check using the in
operator to properly verify that the key is actually removed from the result
object.
In `@packages/ai-core/src/AiSlice.ts`:
- Around line 1368-1377: The addAnalysisResult method in AiSlice.ts is currently
a silent no-op that accepts a UIMessage parameter but performs no operations,
causing existing code that calls this method to fail silently without updating
uiMessages. Either implement the method to properly translate the legacy call by
adding the provided UIMessage to the uiMessages state (similar to how the new
chat transport updates uiMessages), or remove the method entirely and prepare
for a breaking release if this is a public API. Do not leave it as a silent
no-op that discards input without error.
---
Outside diff comments:
In `@packages/ai-config/src/migration/AnalysisSession-v0.26.0.ts`:
- Around line 84-152: The code synthesizing messages from analysisResults does
not preserve the errorMessage field from the result object. This causes
downstream code in getChatRequestErrorMessage() to lose error information when
reading message.metadata.sqlrooms.errorMessage. Extract the errorMessage from
each result in the loop (similar to how prompt and streamMessage are extracted),
and when creating synthesized messages, add a metadata object with the structure
metadata.sqlrooms.errorMessage to preserve the error information for downstream
processing.
In `@packages/ai-core/src/AiSlice.ts`:
- Around line 281-297: The session cleanup logic (running
cleanupPendingUiMessages, fixIncompleteToolCalls, and resetting isRunning to
false) is only applied during slice construction but not when setConfig() is
called. This means callers can bypass normalization by restoring config through
the public setter, reintroducing problematic state like isRunning: true or
incomplete tool calls. Extract the session normalization logic from the slice
constructor (the mapping over params.config.sessions that calls
cleanupPendingUiMessages and fixIncompleteToolCalls) into a shared helper
function or method, then apply this normalizer to the sessions parameter in both
the constructor code shown and the setConfig() method to ensure consistent
normalization regardless of how config is ingested.
🪄 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: ff8e221a-1a04-467f-af05-ff07ec7dde57
📒 Files selected for processing (34)
apps/sqlrooms-cli-ui/src/components/AssistantChatContainer.tsxdocs/upgrade-guide.mdpackages/ai-config/README.mdpackages/ai-config/src/AiSliceConfig.tspackages/ai-config/src/index.tspackages/ai-config/src/migration/AnalysisSession-v0.26.0.tspackages/ai-config/src/schema/ChatSessionSchema.tspackages/ai-core/README.mdpackages/ai-core/__tests__/AiSlice.modelSelection.test.tspackages/ai-core/__tests__/ChatHistoryView.test.tsxpackages/ai-core/__tests__/chatTurns.test.tspackages/ai-core/__tests__/contextSelection.test.tspackages/ai-core/__tests__/migration.test.tspackages/ai-core/src/AiSlice.tspackages/ai-core/src/chatTransport.tspackages/ai-core/src/chatTurns.tspackages/ai-core/src/components/AnalysisResult.tsxpackages/ai-core/src/components/Chat.tsxpackages/ai-core/src/components/ChatHeader.tsxpackages/ai-core/src/components/ChatHistoryItem.tsxpackages/ai-core/src/components/ChatHistoryView.tsxpackages/ai-core/src/components/ChatMessagesContainer.tsxpackages/ai-core/src/constants.tspackages/ai-core/src/contextSelection.tspackages/ai-core/src/hooks/useGenerateSessionTitle.tspackages/ai-core/src/index.tspackages/ai-core/src/types.tspackages/ai-core/src/utils.tspackages/ai/README.mdpackages/ai/src/index.tspackages/artifacts/README.mdpackages/artifacts/__tests__/artifactAiSessions.test.tspackages/artifacts/src/ai/artifactAiSessionHelpers.tspackages/artifacts/src/ai/artifactAiSlice.ts
💤 Files with no reviewable changes (2)
- packages/ai-core/src/constants.ts
- packages/ai-core/tests/AiSlice.modelSelection.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7cda404d3
ℹ️ 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 hasLegacyToolData = 'toolAdditionalData' in d; | ||
|
|
||
| return !hasUiMessages || hasLegacyToolData; | ||
| return !hasUiMessages || hasLegacyAnalysisResults || hasLegacyToolData; |
There was a problem hiding this comment.
When upgrading an already-migrated saved session that still has legacy analysisResults (the previous schema stored both uiMessages and analysisResults), this new guard runs migrateFromV0_26_0 just to drop the legacy field. That migration still unconditionally returns prompt: '', so any unsent per-session draft prompt is erased on reload/upgrade even though the session already has uiMessages. Preserve the existing session.prompt when uiMessages is present, and only default it for genuinely pre-chat sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ai-config/src/migration/AnalysisSession-v0.26.0.ts (1)
146-205: 💤 Low valueRedundant condition check.
The
if (needsMessageSynthesis)at line 146 is always true at this point because the!needsMessageSynthesisbranch on line 131 ends withcontinue. This doesn't affect correctness but adds unnecessary nesting.♻️ Suggested simplification
- if (needsMessageSynthesis) { // Create user message for the prompt if (prompt) { synthesizedMessages.push( ... ); } ... - }🤖 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-config/src/migration/AnalysisSession-v0.26.0.ts` around lines 146 - 205, Remove the redundant `if (needsMessageSynthesis)` condition check that wraps the message synthesis logic, since the opposite branch ends with `continue` and control will never reach this point when needsMessageSynthesis is false. Unindent all the code that creates the user message (using synthesizedMessages.push with withLegacyResultMetadata), builds the assistantParts array by iterating over parts, and pushes the assistant message to synthesizedMessages by one level of indentation.
🤖 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/ai-core/src/utils.ts`:
- Around line 255-271: The normalizeAiSession function contains redundant
processing: cleanupPendingUiMessages already calls fixIncompleteToolCalls
internally, but then the result is passed through fixIncompleteToolCalls again
in lines 259-263. Remove the second unnecessary call to fixIncompleteToolCalls
and instead use the uiMessages directly from the cleaned result of
cleanupPendingUiMessages, simplifying the logic to avoid double-processing the
same messages.
---
Nitpick comments:
In `@packages/ai-config/src/migration/AnalysisSession-v0.26.0.ts`:
- Around line 146-205: Remove the redundant `if (needsMessageSynthesis)`
condition check that wraps the message synthesis logic, since the opposite
branch ends with `continue` and control will never reach this point when
needsMessageSynthesis is false. Unindent all the code that creates the user
message (using synthesizedMessages.push with withLegacyResultMetadata), builds
the assistantParts array by iterating over parts, and pushes the assistant
message to synthesizedMessages by one level of indentation.
🪄 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: 32fc4b75-785e-4c1c-bfa4-dc47f68cdfef
📒 Files selected for processing (8)
docs/upgrade-guide.mdpackages/ai-config/src/migration/AnalysisSession-v0.26.0.tspackages/ai-core/__tests__/AiSlice.modelSelection.test.tspackages/ai-core/__tests__/chatTurns.test.tspackages/ai-core/__tests__/migration.test.tspackages/ai-core/src/AiSlice.tspackages/ai-core/src/chatTurns.tspackages/ai-core/src/utils.ts
✅ Files skipped from review due to trivial changes (1)
- docs/upgrade-guide.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ai-core/src/chatTurns.ts
- packages/ai-core/tests/migration.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cd19f2907
ℹ️ 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".
| export type { | ||
| AiRunContext, | ||
| AiRunContextItem, | ||
| AnalysisSessionSchema, | ||
| } from '@sqlrooms/ai-config'; |
There was a problem hiding this comment.
Existing apps that import type AnalysisSessionSchema from @sqlrooms/ai-core lose that public type in this barrel even though the migration docs say the analysis-named APIs remain compatibility aliases. Please keep re-exporting the deprecated type alongside the new chat names so downstream TypeScript consumers do not break during the migration window.
Useful? React with 👍 / 👎.
addresses #198
Summary
uiMessagesSummary by CodeRabbit
Release Notes
uiMessages, deriving chat turns for the chat timeline and related search.