Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a comprehensive AI chat feature integrating OpenRouter API for analytics queries. Adds backend service layer (AiService, AiChatService), REST endpoints for chat streaming and management, frontend chat UI component, database schema, and supporting infrastructure (DTOs, entity, module, localization, dependencies). Changes
Sequence DiagramsequenceDiagram
participant Client as Client (Web)
participant Controller as AiController
participant AiSvc as AiService
participant OpenRouter as OpenRouter API
participant ChatSvc as AiChatService
participant DB as Database
Client->>Controller: POST /ai/:pid/chat<br/>(messages, timezone)
activate Controller
Controller->>Controller: Validate auth & project access
Controller->>Controller: Check billing status
Controller->>Controller: Setup SSE response stream
Controller->>AiSvc: chat(project, messages, timezone)
activate AiSvc
AiSvc->>AiSvc: Build system prompt & tools
AiSvc->>OpenRouter: POST (streaming)<br/>with tools & messages
activate OpenRouter
OpenRouter-->>AiSvc: Stream: text chunks
AiSvc->>AiSvc: Normalize tool_calls
AiSvc-->>Controller: Emit text events
Controller-->>Client: SSE: text data
OpenRouter-->>AiSvc: Stream: tool_call
AiSvc->>AiSvc: Execute tool<br/>(getData, getGoalStats, etc.)
AiSvc-->>Controller: Emit tool-call event
Controller-->>Client: SSE: tool-call data
AiSvc->>AiSvc: Process tool results
AiSvc-->>Controller: Emit tool-result event
Controller-->>Client: SSE: tool-result data
OpenRouter-->>AiSvc: Stream complete
deactivate OpenRouter
AiSvc-->>Controller: Emit done signal
Controller-->>Client: SSE: done
deactivate AiSvc
Controller->>ChatSvc: create/update chat<br/>(messages, response)
activate ChatSvc
ChatSvc->>DB: INSERT/UPDATE ai_chat
deactivate ChatSvc
Controller-->>Client: Response closed
deactivate Controller
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
web/app/pages/Project/View/components/ProjectSidebar.tsx (1)
16-32: Sidebar AI tab wiring and styling are consistent, but rely on the same self‑hosted tab id behaviorThe new
ICON_COLORS.aimapping, hover/active class tweaks, and standalone Ask AI block above the grouped tabs all look consistent with the existing sidebar design. In cloud, resolvingaskAiTabfromtabsand updating thetabsearch param works as intended.Note that the self‑hosted case shares the same
PROJECT_TABS.airuntimeundefinedissue called out inViewProject.tsx(tabs definition): in self‑hosted builds the Ask AI entry will still render here but never become the active tab. Once the tab construction is gated to cloud only (per that comment), this sidebar logic will follow automatically.Also applies to: 57-83, 161-208, 225-266, 281-288
🧹 Nitpick comments (12)
backend/apps/cloud/src/ai/entity/ai-chat.entity.ts (2)
33-41: Consider adding an index on theprojectrelation for query performance.When listing chats for a project (likely a common query pattern), an index on the
projectIdforeign key column would improve performance. TypeORM doesn't automatically create indexes on foreign keys in all database engines.+import { Index } from 'typeorm' @Entity('ai_chat') +@Index(['project']) export class AiChat {Alternatively, this can be handled in the migration if you prefer to keep the entity cleaner.
29-31: ThemessagesJSON column could grow unbounded.Consider whether there should be a limit on the number of messages per chat, or if older messages should be truncated. Unbounded JSON columns can impact query performance and storage over time. This may be handled at the application layer, but it's worth considering adding documentation or validation.
web/app/api/index.ts (3)
1972-1985: Use PATCH or PUT instead of POST for update operations.The
updateAIChatfunction uses POST for updating an existing resource, which violates REST conventions. Other update functions in this file (e.g.,updateAlert,updateGoal,updateProject) use PUT or PATCH.export const updateAIChat = async ( pid: string, chatId: string, data: { messages?: AIChatMessage[]; name?: string }, ): Promise<AIChat> => { return api - .post(`ai/${pid}/chats/${chatId}`, data, { - headers: { Authorization: getAccessToken() ? `Bearer ${getAccessToken()}` : '' }, - }) + .patch(`ai/${pid}/chats/${chatId}`, data) .then((response): AIChat => response.data) .catch((error) => { throw _isEmpty(error.response?.data?.message) ? error.response?.data : error.response?.data?.message }) }Note: This also requires updating the backend controller to handle PATCH requests for this endpoint.
1860-1866: Thewhile (true)loop without explicit break condition could hang if the stream stalls.If the server connection stays open but stops sending data (stalled connection), this loop will wait indefinitely. Consider adding a timeout mechanism or ensuring the server properly closes the connection.
Verify that the backend properly closes the SSE connection on completion/error, and consider whether client-side timeouts are needed for resilience.
1893-1895: Silent JSON parsing failures may hide legitimate errors.While ignoring incomplete JSON chunks during streaming is necessary, completely swallowing all parse errors could hide issues with malformed server responses. Consider logging in development mode or distinguishing between incomplete chunks and actual parse errors.
} catch (e) { - // Ignore parsing errors for incomplete JSON + // Log parsing errors in development for debugging + if (process.env.NODE_ENV === 'development') { + console.debug('SSE JSON parse error (may be incomplete chunk):', data, e) + } }web/app/pages/Project/View/ViewProject.tsx (1)
4320-4321: AskAIView mounting with projectId is straightforwardRendering
<AskAIView projectId={id} />whenactiveTab === PROJECT_TABS.aicorrectly scopes the AI experience to the current project and reuses the standardidfromuseCurrentProject().You could mirror
CaptchaViewand lazily load AskAIView viaReact.lazyto avoid pulling the entire AI UI bundle into initial dashboard loads if code‑size becomes a concern.web/app/pages/Project/AskAI/AIChart.tsx (1)
29-40: AIChart implementation is solid; consider a couple of small robustness tweaksOverall this component is well‑structured:
- Clean separation between pie/donut and x‑series charts with appropriate data validation and early returns.
- Sensible color palette and percentage/value formatting in tooltips.
- Dynamic axis configuration (including tick density, rotation, and date formatting) is calibrated to the data volume.
Two small robustness improvements you might consider:
Guard against charts with no data series
Right now, if
chart.datahasxbut no additional numeric keys,seriesKeyswill be empty and you’ll render an empty chart frame. You could treat this as “no chart” instead:const xData = chart.data.x as string[] const seriesKeys = _filter(_keys(chart.data), (key) => key !== 'x' && key !== 'labels' && key !== 'values')
- if (seriesKeys.length === 0) {
- return {}
- }
and in the render guard, mirror that by returning `null` when `seriesKeys.length === 0`.
Document/assume non‑negative series values
calculateOptimalTicksfixesy.min = 0and computes ticks assuming non‑negative values. If AI‑generated charts ever include negative‑only data,upperBoundbecomes ≤ 0 andtickscan be empty. If you expect only counts or durations, that’s fine, but a short comment noting the non‑negative assumption nearcalculateOptimalTickswould make the intent explicit for future maintainers.Also applies to: 41-85, 87-107, 109-213, 215-305, 308-328
web/app/pages/Project/AskAI/AskAIView.tsx (3)
453-453: Usesubstringinstead of deprecatedsubstr.
String.prototype.substris deprecated. Usesubstringinstead for better compatibility.- const generateMessageId = () => `msg-${Date.now()}-${Math.random().toString(36).substr(2, 9)}` + const generateMessageId = () => `msg-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`
759-772: Hardcoded time strings should use i18n.The
formatRelativeTimefunction uses hardcoded English strings ('Just now','m','h','d') while the rest of the component properly usesuseTranslation. Consider using translated strings for consistency.const formatRelativeTime = (dateStr: string) => { const date = new Date(dateStr) const now = new Date() const diffMs = now.getTime() - date.getTime() const diffMins = Math.floor(diffMs / 60000) const diffHours = Math.floor(diffMs / 3600000) const diffDays = Math.floor(diffMs / 86400000) - if (diffMins < 1) return 'Just now' - if (diffMins < 60) return `${diffMins}m` - if (diffHours < 24) return `${diffHours}h` - if (diffDays < 7) return `${diffDays}d` + if (diffMins < 1) return t('askAi.time.justNow') + if (diffMins < 60) return t('askAi.time.minutes', { count: diffMins }) + if (diffHours < 24) return t('askAi.time.hours', { count: diffHours }) + if (diffDays < 7) return t('askAi.time.days', { count: diffDays }) return date.toLocaleDateString() }
1067-1067: Hardcoded "Load more" text should use i18n.- Load more + {t('askAi.loadMore')}backend/apps/cloud/src/ai/ai.controller.ts (2)
246-250: Add validation for numeric query parameters.
parseInton invalid strings returnsNaN, which could cause unexpected behavior in the service layer. Consider adding validation or using a more defensive approach.const chats = await this.aiChatService.findRecentByProject( pid, uid, - limit ? parseInt(limit, 10) : 5, + limit ? Math.max(1, parseInt(limit, 10) || 5) : 5, )Similarly for
getAllChats:const result = await this.aiChatService.findAllByProject( pid, uid, - skip ? parseInt(skip, 10) : 0, - take ? parseInt(take, 10) : 20, + skip ? Math.max(0, parseInt(skip, 10) || 0) : 0, + take ? Math.min(100, Math.max(1, parseInt(take, 10) || 20)) : 20, )Adding an upper bound on
takealso prevents potential DoS via large pagination requests.
372-415: Consider using PATCH instead of POST for the update endpoint.REST conventions typically use
PATCH(orPUT) for update operations. UsingPOSTfor updates may confuse API consumers.@ApiBearerAuth() -@Post(':pid/chats/:chatId') +@Patch(':pid/chats/:chatId') @Auth() @ApiOperation({ summary: 'Update an AI chat' })This would require importing
Patchfrom@nestjs/common.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
backend/package-lock.jsonis excluded by!**/package-lock.jsonweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
backend/.env.example(1 hunks)backend/apps/cloud/src/ai/ai-chat.service.ts(1 hunks)backend/apps/cloud/src/ai/ai.controller.ts(1 hunks)backend/apps/cloud/src/ai/ai.module.ts(1 hunks)backend/apps/cloud/src/ai/ai.service.ts(1 hunks)backend/apps/cloud/src/ai/dto/chat.dto.ts(1 hunks)backend/apps/cloud/src/ai/entity/ai-chat.entity.ts(1 hunks)backend/apps/cloud/src/app.module.ts(2 hunks)backend/migrations/mysql/2025_12_07_ai_chat.sql(1 hunks)backend/package.json(3 hunks)web/app/api/index.ts(1 hunks)web/app/lib/constants/index.ts(1 hunks)web/app/pages/Project/AskAI/AIChart.tsx(1 hunks)web/app/pages/Project/AskAI/AskAIView.tsx(1 hunks)web/app/pages/Project/AskAI/index.tsx(1 hunks)web/app/pages/Project/View/ViewProject.tsx(5 hunks)web/app/pages/Project/View/components/ProjectSidebar.tsx(6 hunks)web/package.json(1 hunks)web/public/locales/en.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
backend/apps/cloud/src/ai/ai.module.ts (1)
backend/apps/cloud/src/app.module.ts (1)
Module(91-105)
web/app/pages/Project/View/components/ProjectSidebar.tsx (2)
web/app/lib/constants/index.ts (1)
PROJECT_TABS(423-425)web/app/ui/Text.tsx (1)
Text(51-75)
web/app/api/index.ts (2)
web/app/utils/accessToken.ts (1)
getAccessToken(11-23)web/app/lib/constants/index.ts (1)
API_URL(338-338)
backend/apps/cloud/src/ai/ai.controller.ts (2)
backend/apps/cloud/src/ai/dto/chat.dto.ts (3)
ChatDto(29-54)CreateChatDto(56-73)UpdateChatDto(75-94)backend/apps/cloud/src/ai/ai.service.ts (1)
chat(111-142)
backend/apps/cloud/src/ai/ai-chat.service.ts (2)
backend/apps/cloud/src/ai/entity/ai-chat.entity.ts (1)
ChatMessage(14-17)backend/apps/cloud/src/ai/ai.controller.ts (1)
chat(48-223)
web/app/pages/Project/View/ViewProject.tsx (1)
web/app/lib/constants/index.ts (1)
PROJECT_TABS(423-425)
🪛 ast-grep (0.40.0)
web/app/pages/Project/AskAI/AskAIView.tsx
[warning] 333-333: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
web/app/pages/Project/AskAI/AskAIView.tsx
[error] 334-334: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (19)
backend/.env.example (1)
100-102: LGTM!The new
OPENROUTER_API_KEYenvironment variable is appropriately placed under the Swetrix Enterprise (Cloud) section with a clear comment explaining its purpose.web/package.json (1)
76-77: LGTM!The
use-stick-to-bottomdependency is appropriate for implementing auto-scroll behavior in the AI chat interface, and follows the existing versioning conventions used in the project.backend/package.json (1)
29-29: LGTM!The new dependencies (
@ai-sdk/openai,ai, andzod) are appropriate for implementing AI chat functionality with the Vercel AI SDK ecosystem. Thezodlibrary provides robust schema validation for AI tool inputs.Also applies to: 51-51, 93-94
web/public/locales/en.json (1)
1742-1761: LGTM!The localization strings are comprehensive and well-written. The inclusion of the
disclaimerkey ("AI responses may not always be accurate. Verify important data.") is a good practice for setting user expectations about AI-generated content.web/app/api/index.ts (1)
1813-1825: LGTM on the interface definitions.The
AIChatMessageandAIStreamCallbacksinterfaces are well-designed with appropriate optional callbacks for different event types (tool calls, reasoning, etc.).web/app/pages/Project/AskAI/index.tsx (1)
1-1: AskAI index re-export is correct and minimalRe-exporting the default from
./AskAIViewkeeps the route entrypoint clean and matches the../AskAIimport usage.backend/apps/cloud/src/app.module.ts (1)
24-24: AiModule integration into AppModule looks correctImporting
AiModuleand adding it tomodulesensures AI endpoints and services are available in the cloud app; no structural issues stand out.Also applies to: 83-83
web/app/lib/constants/index.ts (1)
410-421: AI tab constant added only for production tabsAdding
ai: 'ai'toPRODUCTION_PROJECT_TABScorrectly scopes the AI tab to the cloud/production tab set; the self-hosted tab set remains unchanged, with behavioral implications handled in the ViewProject/ProjectSidebar logic.backend/apps/cloud/src/ai/ai.module.ts (1)
1-24: AiModule composition is consistent with NestJS patternsThe module correctly registers the
AiChatentity, AI services, and controller, and imports the needed domain modules; no structural or DI issues are apparent.web/app/pages/Project/View/ViewProject.tsx (1)
3477-3483: Toolbar/header gating for AI tab is reasonableSkipping the standard dashboard header and controls when
activeTabis the AI tab (and alerts) avoids rendering analytics‑specific filters around a fundamentally different view; this conditional looks consistent with the layout needs of Ask AI.backend/migrations/mysql/2025_12_07_ai_chat.sql (1)
1-16: AI chat migration schema is coherentThe
ai_chattable definition (keys, JSON payload, timestamps, and foreign keys withON DELETE CASCADE) matches a typical chat‑history model and should perform well for project/user and recent‑history queries.backend/apps/cloud/src/ai/dto/chat.dto.ts (1)
1-94: DTOs are well-structured with proper validation.The DTO definitions follow NestJS best practices with appropriate use of
class-validatordecorators andclass-transformerfor nested object validation. TheValidateNested({ each: true })combined with@Type(() => ChatMessageDto)ensures proper transformation and validation of nested message arrays.web/app/pages/Project/AskAI/AskAIView.tsx (2)
326-347: Sanitization is applied before using dangerouslySetInnerHTML - acceptable pattern.The static analysis warning is a false positive in this context. The
renderMarkdownfunction (lines 124-135) properly sanitizes HTML usingsanitize-htmlbefore injection, which is the recommended approach when rendering user-controlled or AI-generated markdown content. The sanitization configuration restricts allowed tags and attributes appropriately.
541-551: Initialization effect correctly uses ref guard to prevent re-execution.The
hasInitializedRefpattern ensures the initialization logic runs only once, which is the correct behavior for loading initial state from URL parameters. The dependencies are listed for completeness per React lint rules, but the ref guard prevents re-runs.backend/apps/cloud/src/ai/ai.service.ts (3)
28-92: Well-implemented stream patching for OpenRouter compatibility.The
patchedFetchwrapper correctly handles a known issue with some OpenRouter models returning emptytypefields in tool_calls. The stream handling with proper cleanup and error fallback is robust.
94-142: Service initialization and chat method are well-structured.The service properly initializes the OpenRouter client with the patched fetch and exposes a clean
chatmethod. The logging provides good observability without exposing sensitive data. ThestreamTextconfiguration withmaxSteps: 10is a reasonable limit for tool call iterations.
1011-1020: No changes needed. Thetimezoneparameter is properly sanitized byanalyticsService.getSafeTimezone()before being used in SQL strings. This method validates the timezone against the IANA timezone database usingdayjs.tz(), rejecting invalid timezones and falling back toDEFAULT_TIMEZONE. All call sites ingetAnalytics(),getGoalStats(), andgetFunnelStats()sanitize the timezone before passing it togetTimeBucketSelect(), preventing SQL injection attacks.Likely an incorrect or invalid review comment.
backend/apps/cloud/src/ai/ai.controller.ts (1)
43-223: Robust SSE streaming implementation with proper error handling.The chat endpoint correctly sets up SSE headers, handles stream errors gracefully (both during iteration and as error events), and properly ends the response. The
hasContentflag ensures appropriate error messaging based on whether partial content was delivered. Good practice to checkres.headersSentbefore throwing exceptions.backend/apps/cloud/src/ai/ai-chat.service.ts (1)
1-134: Clean repository service implementation.The
AiChatServicefollows NestJS/TypeORM best practices with proper dependency injection and query builder usage. ThegenerateChatNamehelper provides good UX by auto-naming chats based on the first user message. The access verification logic correctly handles both user-specific and shared (null userId) chats.
1 participant
Changes
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.