Fix chat name generation #1719
Fix chat name generation #1719
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR refactors chat name generation to return a structured object Changes
Sequence DiagramsequenceDiagram
participant Client as Frontend Client
participant API as API Endpoint
participant Controller as MCP Chat Controller
participant Service as AI Chat Service
participant LLM as Language Model
participant DB as Database
Client->>API: POST /chat-name { chatId }
API->>Controller: route to generateChatName()
Controller->>Controller: fetch chatHistory & projectId
rect rgb(200, 220, 240)
Note over Service,LLM: Schema-based Generation
Controller->>Service: generateChatName(chatHistory, projectId)
Service->>Service: sanitizeMessagesForChatName(history)
Service->>LLM: generateObject(messages, schema)
LLM-->>Service: { name: string, isGenerated: boolean }
end
Service-->>Controller: GeneratedChatName object
alt isGenerated === true
Controller->>DB: updateChatName(chatId, userId, name)
DB-->>Controller: ✓ updated
else isGenerated === false
Note over Controller: Skip DB update, use default
end
Controller-->>API: { name: string, isGenerated: boolean }
API-->>Client: { name, isGenerated }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Comment |
… and maintainability.
…tings in chat name generation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| 'role' in msg && | ||
| msg.role === 'user', | ||
| ); | ||
| const generated = await generateChatName(chatHistory, projectId); |
There was a problem hiding this comment.
The filtering is removed, having only user messages for generation is not enough
![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 (4)
packages/server/api/src/app/ai/chat/utils.ts (1)
67-95: Consider using|| nullfor empty-string handling clarity.The nullish coalescing operator (
??) on lines 70 and 91 only convertsnull/undefinedtonull, not empty strings. The code works correctly because line 101 handles empty strings with a truthiness check, but using|| nullinextractTextwould be more explicit about the intent to treat empty strings as "no content."const extractText = (content: ModelMessage['content']): string | null => { if (typeof content === 'string') { const text = content.trim(); - return text ?? null; + return text || null; } if (Array.isArray(content)) { const merged = (content as Array<unknown>) .reduce<string[]>((acc, part) => { if ( part && typeof part === 'object' && 'type' in (part as Record<string, unknown>) ) { const p = part as { type?: string; text?: string }; if (p.type === 'text' && typeof p.text === 'string') { acc.push(p.text); } } return acc; }, []) .join('\n') .trim(); - return merged ?? null; + return merged || null; } return null; };packages/server/api/test/unit/ai/ai-mcp-chat.controller.test.ts (3)
525-540: Mock should include name property for consistency.The mock at line 525-527 returns only
{ isGenerated: false }without anameproperty. While the test expects the controller to provide the fallback "New Chat" name, the mock should ideally match the actual service contract for consistency.Consider updating the mock to match the expected service response shape:
- (generateChatName as jest.Mock).mockResolvedValue({ - isGenerated: false, - }); + (generateChatName as jest.Mock).mockResolvedValue({ + isGenerated: false, + name: undefined, + });This makes it clearer that the service may return
undefinedfor the name whenisGeneratedis false, and the controller handles the fallback.
542-558: Test description doesn't match implementation.The test description mentions "if LLM returns empty string", but the mock at line 546 returns
{ isGenerated: false }without any name property. The test would be more accurate if either:
- The description is updated to match the mock behavior, or
- The mock explicitly returns an empty string:
{ isGenerated: false, name: '' }Update the mock to match the test description:
- (generateChatName as jest.Mock).mockResolvedValue({ isGenerated: false }); + (generateChatName as jest.Mock).mockResolvedValue({ + isGenerated: false, + name: '' + });Or update the test description to: "should return 'New Chat' when isGenerated is false"
353-622: Consider adding test coverage for edge cases.The test suite covers the main scenarios well, but consider adding tests for the following edge cases to ensure robustness:
- Very long generated names: Verify truncation behavior if the LLM generates an excessively long name (e.g., >100 characters).
- Generated names with special characters: Verify sanitization of potentially problematic characters (e.g., newlines, tabs, control characters).
- Inconsistent service response: Verify behavior when
isGeneratedis true butnameis empty, undefined, or whitespace-only.Example test case to add:
it('should handle excessively long generated names', async () => { const longName = 'A'.repeat(200); (getConversation as jest.Mock).mockResolvedValue({ chatHistory: [{ role: 'user', content: 'Test' }], }); (generateChatName as jest.Mock).mockResolvedValue({ isGenerated: true, name: longName, }); await postHandler( { ...mockRequest, body: { chatId: 'test-chat-id' } } as FastifyRequest, mockReply as unknown as FastifyReply, ); // Verify truncation or rejection expect(mockReply.code).toHaveBeenCalledWith(200); expect(mockReply.send).toHaveBeenCalledWith({ isGenerated: true, name: expect.stringMatching(/^.{1,100}$/), // Expect max 100 chars }); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ai-prompts/chat-name.txt(1 hunks)packages/react-ui/src/app/features/ai/lib/ai-assistant-chat-history-api.ts(2 hunks)packages/react-ui/src/app/features/ai/lib/assistant-ui-chat-hook.ts(1 hunks)packages/server/api/src/app/ai/chat/ai-chat.service.ts(3 hunks)packages/server/api/src/app/ai/chat/ai-mcp-chat.controller.ts(2 hunks)packages/server/api/src/app/ai/chat/utils.ts(1 hunks)packages/server/api/test/unit/ai/ai-mcp-chat.controller.test.ts(11 hunks)packages/server/api/test/unit/ai/utils.test.ts(2 hunks)packages/shared/src/lib/ai/chat/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx,js,jsx}: Indentation: 2 spaces for TypeScript/JavaScript
Braces required for all control blocks, even single-line
One space between keywords and parentheses:if (condition) {
Use camelCase for variables and functions
Use PascalCase for classes and types
Use UPPER_SNAKE_CASE for constants
Use lowercase with hyphens for file names (e.g.,user-profile.ts)
Preferconstoverletorvarin TypeScript/JavaScript
Prefer arrow functions for callbacks and functional components in TypeScript/JavaScript
Files:
packages/shared/src/lib/ai/chat/index.tspackages/server/api/test/unit/ai/utils.test.tspackages/server/api/src/app/ai/chat/utils.tspackages/server/api/src/app/ai/chat/ai-mcp-chat.controller.tspackages/server/api/src/app/ai/chat/ai-chat.service.tspackages/react-ui/src/app/features/ai/lib/ai-assistant-chat-history-api.tspackages/server/api/test/unit/ai/ai-mcp-chat.controller.test.tspackages/react-ui/src/app/features/ai/lib/assistant-ui-chat-hook.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Use types and interfaces where appropriate in TypeScript
Use explicit return types for exported functions in TypeScript
Files:
packages/shared/src/lib/ai/chat/index.tspackages/server/api/test/unit/ai/utils.test.tspackages/server/api/src/app/ai/chat/utils.tspackages/server/api/src/app/ai/chat/ai-mcp-chat.controller.tspackages/server/api/src/app/ai/chat/ai-chat.service.tspackages/react-ui/src/app/features/ai/lib/ai-assistant-chat-history-api.tspackages/server/api/test/unit/ai/ai-mcp-chat.controller.test.tspackages/react-ui/src/app/features/ai/lib/assistant-ui-chat-hook.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{tsx,ts}: Frontend tech stack must strictly use: React 18, Zustand, react-query v5, shadcn, and Axios withqspackage for query strings
Follow best practices for React hooks
Prefer small, composable components and extract helper functions where possible
Usecnutility to group tailwind classnames in React components
Files:
packages/shared/src/lib/ai/chat/index.tspackages/server/api/test/unit/ai/utils.test.tspackages/server/api/src/app/ai/chat/utils.tspackages/server/api/src/app/ai/chat/ai-mcp-chat.controller.tspackages/server/api/src/app/ai/chat/ai-chat.service.tspackages/react-ui/src/app/features/ai/lib/ai-assistant-chat-history-api.tspackages/server/api/test/unit/ai/ai-mcp-chat.controller.test.tspackages/react-ui/src/app/features/ai/lib/assistant-ui-chat-hook.ts
🧬 Code graph analysis (5)
packages/server/api/test/unit/ai/utils.test.ts (1)
packages/server/api/src/app/ai/chat/utils.ts (1)
sanitizeMessagesForChatName(61-104)
packages/server/api/src/app/ai/chat/ai-mcp-chat.controller.ts (1)
packages/server/api/src/app/ai/chat/ai-chat.service.ts (2)
generateChatName(102-126)updateChatName(128-142)
packages/server/api/src/app/ai/chat/ai-chat.service.ts (3)
packages/shared/src/lib/ai/chat/index.ts (2)
GeneratedChatName(133-136)GeneratedChatName(137-137)packages/server/api/src/app/ai/chat/utils.ts (1)
sanitizeMessagesForChatName(61-104)packages/openops/src/lib/ai/providers.ts (1)
isLLMTelemetryEnabled(136-138)
packages/react-ui/src/app/features/ai/lib/ai-assistant-chat-history-api.ts (2)
packages/react-ui/src/app/lib/api.ts (1)
api(33-97)packages/shared/src/lib/ai/chat/index.ts (2)
GeneratedChatName(133-136)GeneratedChatName(137-137)
packages/react-ui/src/app/features/ai/lib/assistant-ui-chat-hook.ts (1)
packages/react-ui/src/app/features/ai/lib/ai-assistant-chat-history-api.ts (1)
aiAssistantChatHistoryApi(4-24)
🔇 Additional comments (17)
packages/react-ui/src/app/features/ai/lib/assistant-ui-chat-hook.ts (1)
246-254: LGTM! Clean handling of the new structured response.The conditional retry logic is well-implemented: clearing
hasAttemptedNameGenerationRefwhenisGeneratedis false allows the system to retry name generation on subsequent messages when more context becomes available.ai-prompts/chat-name.txt (1)
1-13: Well-structured prompt with clear output contract.The prompt clearly defines the expected behavior and aligns with the
GeneratedChatNameschema. The distinction between confident generation (isGenerated: true) and insufficient context (isGenerated: false) enables graceful fallback handling.packages/shared/src/lib/ai/chat/index.ts (1)
132-137: LGTM! Type definition correctly models the response contract.The
GeneratedChatNametype properly represents both successful generation (isGenerated: true, name: string) and fallback cases (isGenerated: false, name: null).packages/server/api/src/app/ai/chat/utils.ts (1)
55-104: Well-designed sanitization utility.The function cleanly separates concerns: role filtering, text extraction, and empty content elimination. The implementation correctly handles both string content and structured array content with text parts.
packages/react-ui/src/app/features/ai/lib/ai-assistant-chat-history-api.ts (1)
2-2: LGTM! API type updated to match new response shape.The import and return type correctly reflect the server-side changes, ensuring type safety across the client-server boundary.
Also applies to: 11-14
packages/server/api/test/unit/ai/utils.test.ts (1)
808-885: Comprehensive test coverage for sanitizeMessagesForChatName.The tests effectively cover the key behaviors: role filtering (dropping system/tool), whitespace trimming, text part merging with newlines, non-text part filtering, and empty result handling.
packages/server/api/src/app/ai/chat/ai-mcp-chat.controller.ts (2)
257-265: Defensive null check ongenerated.nameis appropriate.The condition
generated.isGenerated && generated.nameguards against edge cases where the LLM might returnisGenerated: truewith a null/empty name, even though the prompt instructs otherwise.
241-271: Confirm all consumers of/chat-nameendpoint have been updated for the response shape change.The endpoint response changed from
{ chatName }to{ name, isGenerated }. This is a breaking change, but verification confirms all identified consumers have been updated:ai-assistant-chat-history-api.ts(line 12) already types the response asGeneratedChatName, andassistant-ui-chat-hook.ts(line 247) already accessesresult.isGenerated. No orphaned consumers using the old response shape were found. Ensure this change is documented in release notes if this is a versioned API.packages/server/api/src/app/ai/chat/ai-chat.service.ts (2)
93-100: Schema consistency: Zod schema matches TypeBox definition.The Zod schema
generatedChatNameSchemamirrors the TypeBoxGeneratedChatNametype in the shared package. The.max(100)constraint aligns withMAX_CHAT_NAME_LENGTHin the controller.
102-126: Well-structured refactor to object-based generation.The switch from
generateTexttogenerateObjectwith a strict schema ensures type-safe, structured responses. Message sanitization before LLM invocation reduces noise and improves generation quality.packages/server/api/test/unit/ai/ai-mcp-chat.controller.test.ts (7)
409-414: LGTM!The test correctly verifies that when chatHistory is empty, the controller returns
{ name: 'New Chat', isGenerated: false }without callinggenerateChatNameorupdateChatName. This is the expected short-circuit behavior.
418-446: Test logic appears inconsistent with description.The test description states "should return 'New Chat' when chatHistory has no user messages", but the test still calls
generateChatNamewith the chatHistory (line 441-444). This suggests that:
- The controller now always calls
generateChatNamewhen chatHistory is non-empty (regardless of user message presence)- The service is responsible for returning
{ isGenerated: false }when no valid content existsHowever, the mock at line 429 returns
{ isGenerated: false }without anameproperty, which could mask bugs if the service doesn't consistently return this shape.Consider adding an assertion to verify the service received the expected chatHistory:
expect(generateChatName).toHaveBeenCalledWith( chatHistory, 'test-project-id', ); +// Verify chatHistory contains no user messages +expect(chatHistory.filter(m => m.role === 'user')).toHaveLength(0);Also verify that the actual service implementation handles missing
nameproperty correctly whenisGeneratedis false.
448-483: LGTM!The test correctly verifies that the controller passes the raw chatHistory without filtering invalid or malformed messages. The responsibility for validation and filtering has been moved to the service layer, which is a good separation of concerns.
485-516: LGTM!The test correctly verifies that
generateChatNamereceives the complete chatHistory including all user and assistant messages, and thatupdateChatNameis called with the generated name.
560-588: LGTM!The test correctly verifies that when
isGeneratedis true with a valid name, the controller callsupdateChatNameto persist it and returns the expected response format.
590-621: LGTM!The error handling tests correctly verify that both
getConversationandgenerateChatNameerrors are caught and return appropriate 500 error responses.
370-373: The service implementation correctly handles full chatHistory through internal sanitization.The
generateChatNamefunction receives the complete message history but immediately sanitizes it viasanitizeMessagesForChatName, which filters messages to keep onlyuserandassistantroles while removingsystemandtoolmessages. This internal filtering is appropriate and handles all message types correctly. The test expectations align with the actual implementation behavior.
There was a problem hiding this comment.
Pull request overview
This PR improves chat name generation by allowing the LLM to indicate when it has insufficient context to generate a meaningful name. Instead of always generating a name (which could be generic), the system now returns a structured response with an isGenerated flag. When false, the system returns "New Chat" and can retry name generation on subsequent LLM responses.
Key Changes:
- Modified chat name generation to return a structured object containing both the name and an
isGeneratedboolean flag - Introduced message sanitization to extract only user/assistant text content for name generation
- Updated retry logic to attempt name generation again when
isGeneratedis false
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/lib/ai/chat/index.ts | Added GeneratedChatName type definition with name and isGenerated fields |
| packages/server/api/src/app/ai/chat/ai-chat.service.ts | Updated generateChatName to use structured output with schema validation and message sanitization |
| packages/server/api/src/app/ai/chat/utils.ts | Added sanitizeMessagesForChatName function to filter and clean messages for name generation |
| packages/server/api/src/app/ai/chat/ai-mcp-chat.controller.ts | Updated controller to handle structured chat name response and conditionally update chat names |
| packages/react-ui/src/app/features/ai/lib/assistant-ui-chat-hook.ts | Modified to reset name generation attempt flag when isGenerated is false |
| packages/react-ui/src/app/features/ai/lib/ai-assistant-chat-history-api.ts | Updated API return type to GeneratedChatName |
| ai-prompts/chat-name.txt | Enhanced prompt to instruct LLM to set isGenerated flag based on context sufficiency |
| packages/server/api/test/unit/ai/utils.test.ts | Added comprehensive tests for sanitizeMessagesForChatName function |
| packages/server/api/test/unit/ai/ai-mcp-chat.controller.test.ts | Updated tests to match new structured response format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const sanitizedMessages: ModelMessage[] = | ||
| sanitizeMessagesForChatName(messages); | ||
|
|
||
| if (isEmpty(sanitizedMessages.length)) { |
There was a problem hiding this comment.
The isEmpty function is being called with sanitizedMessages.length (a number), but it likely expects an array or object. This should be if (sanitizedMessages.length === 0) or if (isEmpty(sanitizedMessages)).
| if (isEmpty(sanitizedMessages.length)) { | |
| if (sanitizedMessages.length === 0) { |
Greptile OverviewGreptile SummaryThis PR improves chat name generation by letting the LLM decide if it has sufficient context to generate a meaningful name. Instead of always generating a name (which might be generic), the LLM now returns a structured response with Key changes:
Critical issue found:
Confidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant UI as Frontend
participant API as Chat Controller
participant Service as AI Chat Service
participant LLM as Language Model
UI->>API: POST /chat-name { chatId }
API->>Service: getConversation(chatId)
Service-->>API: chatHistory[]
alt chatHistory empty
API-->>UI: { name: "New Chat", isGenerated: false }
else chatHistory has messages
API->>Service: generateChatName(chatHistory, projectId)
Service->>Service: sanitizeMessagesForChatName(messages)
Service->>LLM: generateObject(sanitizedMessages, schema)
LLM-->>Service: { name, isGenerated }
Service-->>API: GeneratedChatName
alt isGenerated && name
API->>Service: updateChatName(chatId, name)
end
API-->>UI: { name, isGenerated }
end
UI->>UI: if !isGenerated, reset flag to retry later
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
| return { name: null, isGenerated: false }; | ||
| } | ||
|
|
||
| const result = await generateObject({ |
There was a problem hiding this comment.
As LLMs don't always generate correct structured output, you can make it a bit more reliable using the approach below
| return { name: null, isGenerated: false }; | ||
| } | ||
|
|
||
| const result = await generateObject({ |
There was a problem hiding this comment.
Also, you could wrap in try catch and return {isGenerated: false} in case there is an error and log the error
| .max(100) | ||
| .nullable() | ||
| .describe('Conversation name or null if it was not generated'), | ||
| isGenerated: z.boolean().describe('Whether the name was generated or not'), |
There was a problem hiding this comment.
@rSnapkoOpenOps why do you need isGenerated?
you can deduce that from checking if (name). The less logic the LLM needs to do, the better.
There was a problem hiding this comment.
Yes, I've tried that, but ai still adds dummy names
…gic and log errors
|
Fixes OPS-3173.
Deployed on UX
The idea is to let LLM decided whether it has enough context or no. In case AI decides that name is generated we set it as chat name. In case there is no enough data, we wait for the next LLM response and try to generate the name once again
Summary by CodeRabbit
New Features
isGeneratedflag to indicate whether a name was successfully generated or a default was used.Bug Fixes
Chores
chatNameto includenameandisGeneratedfields for better clarity on generation status.✏️ Tip: You can customize this high-level summary in your review settings.