feat(backend): add chat REST API with streaming support and DTO validation #189
Conversation
WalkthroughThis pull request introduces a new chat API endpoint by adding a Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant CT as ChatController
participant CS as ChatService
participant CPS as ChatProxyService
participant R as Response
C->>CT: POST /api/chat (chatDto)
CT->>CS: saveMessage(user message)
alt Streaming Enabled
CT->>R: Set streaming headers
CT->>CPS: streamChat(chatDto)
CPS-->>CT: Stream chunk of data
CT->>R: Write chunk (SSE format)
CT->>CS: saveMessage(assistant message)
CT->>R: Write 'DONE' and close stream
else Non-Streaming
CT->>CPS: chatSync(chatDto)
CPS-->>CT: Synchronous response
CT->>CS: saveMessage(assistant message)
CT->>R: Return JSON response
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
backend/src/guard/chat.guard.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-prettier". (The package "eslint-plugin-prettier" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-prettier" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
… improved streaming support
…token and error handling
… remove unused chatStream utility
backend/src/guard/chat.guard.ts
Outdated
|
|
||
| // Handle chat stream request | ||
| if (request.path === '/api/chat' && request.method === 'POST') { | ||
| if (!chatId) { |
There was a problem hiding this comment.
chat id is not require, remove it, only JWT auth is require
![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 (15)
frontend/next.config.mjs (1)
38-47: API proxy configuration properly implemented.The rewrites function correctly configures a proxy for API requests, forwarding requests from
/api/:path*tohttp://localhost:8080/api/:path*. This allows the frontend to communicate with the backend REST API seamlessly.However, consider making the backend URL configurable through environment variables for different deployment environments.
async rewrites() { return [ { source: '/api/:path*', - destination: 'http://localhost:8080/api/:path*', + destination: `${process.env.NEXT_PUBLIC_API_URL || 'http://localhost:8080'}/api/:path*`, }, ]; },backend/src/guard/jwt-auth.guard.ts (2)
29-34: Consider handling additional context types
Currently, the guard checks only for'http'or'graphql'. If you anticipate supporting other transport methods (e.g., WebSockets or RPC), consider adding a fallback or an explicit check to handle them gracefully.
47-50: Limit sensitive payload details in logs
JWT payloads can include personally identifiable information (PII). To preserve user privacy, consider removing, redacting, or partially masking sensitive fields before logging payload details.Also applies to: 57-58
backend/src/interceptor/LoggingInterceptor.ts (2)
30-41: Consider masking GraphQL variables
GraphQL variables may contain sensitive information (e.g., passwords, tokens). Logging them in full could expose confidential details. Consider redacting or selectively logging only non-sensitive fields.
61-75: Use caution when logging full HTTP request bodies
The request body may contain PII or confidential data. Consider obfuscating or redacting sensitive fields to avoid potential privacy or security issues.backend/src/chat/chat.controller.ts (1)
23-23: Leverage user ID in message logging or storage
You’re retrieving the user ID via@GetAuthToken() userId, but it’s never used. Consider using it to associate the message with the correct user or for auditing purposes.frontend/src/hooks/useChatStream.ts (7)
28-45: Event listener logic
The effect properly cleans up the event listener, but review if ignoring the event whenchatIdis truthy is intended.
71-73: Throwing an error for missing token
Consider a more user-friendly approach or re-authentication flow if the token is missing.
75-87: Fetching /api/chat
The request structure is correct. You might consider extracting'/api/chat'into a config or environment variable for flexibility.
89-134: Incomplete streaming TODO
Returningdata.contentfor non-streaming is fine. If streaming is truly needed, complete the commented logic to handle partial updates.
146-153: Assistant message construction
Be aware that usingprev.lengthfor IDs can cause conflicts if messages are removed or reordered.
157-158: Error handling
Consider passing a clearer message or logging error details.
217-217:isStreamingalias
RenamingloadingSubmittoisStreamingmight cause confusion if more states form.backend/src/guard/chat.guard.ts (2)
20-23: Context check
Consider adding stricter typing for theuservariable if possible.
90-92: Extracting token from WebSocket handshake
Ensure the format consistently includes a scheme like "Bearer ".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/src/chat/chat.controller.ts(1 hunks)backend/src/chat/chat.module.ts(2 hunks)backend/src/chat/chat.service.ts(2 hunks)backend/src/chat/dto/chat-rest.dto.ts(1 hunks)backend/src/guard/chat.guard.ts(4 hunks)backend/src/guard/jwt-auth.guard.ts(3 hunks)backend/src/interceptor/LoggingInterceptor.ts(1 hunks)frontend/next.config.mjs(2 hunks)frontend/src/hooks/useChatStream.ts(4 hunks)
🔇 Additional comments (30)
backend/src/chat/dto/chat-rest.dto.ts (1)
1-16: Well-structured DTO with proper validations.The
ChatRestDtoclass is correctly implemented with proper validations using class-validator decorators. The required properties (chatId,message,model) are appropriately marked with@IsString(), and the optionalstreamproperty is correctly marked with@IsBoolean()and@IsOptional()with a default value offalse.This DTO will ensure that all incoming request data is properly validated before being processed by the controller, reducing the risk of invalid data being processed.
backend/src/chat/chat.module.ts (2)
3-3: Appropriate import of the new controller.The ChatController import is correctly added to support the new REST API functionality.
22-22: Controller properly registered in the module.The ChatController is correctly added to the controllers array in the ChatModule, making it available for handling HTTP requests.
frontend/next.config.mjs (1)
12-13: Code simplification in webpack configuration.The code has been simplified by removing unnecessary comments while maintaining the same functionality for the webpack fallback configuration.
backend/src/chat/chat.service.ts (4)
35-40: Correctly implemented synchronous chat method.The
chatSyncmethod is well-implemented to handle non-streaming chat requests. It correctly uses the model provider to get synchronous responses and follows the same pattern as the streaming method.The implementation aligns with the controller's usage shown in the relevant code snippets, where it's used for non-streaming responses.
182-184: Parameter name correction.Fixed the parameter name from
upateChatTitleInputtoupdateChatTitleInput, improving code readability and consistency.
186-187: Query parameter update to match parameter name change.The query parameter is correctly updated to use the fixed parameter name.
190-190: Chat title assignment update to match parameter name change.The chat title assignment is correctly updated to use the fixed parameter name.
frontend/src/hooks/useChatStream.ts (15)
1-3: Imports look consistent
Everything appears properly structured.
7-7: Auth context import
Works fine with the usage in the file.
17-17: Clean destructuring pattern
No issues. The function signature and context usage look good.Also applies to: 23-23, 26-26
46-49: Sync chatId state
Straightforward approach to keep local state in sync with prop changes.
65-70: startChatStream signature
Use of a default parameter to disable streaming by default is a sensible practice.
136-136: New handleChatResponse function
A concise wrapper to separate the chat logic from UI concerns.
138-138: Clearing input
Good practice to reset the input field once the request begins.
139-140: Calling startChatStream
Consistent usage of the newly introduced streaming function.
142-142: Using selectedModel
Straightforward pass-through for the chosen model.
144-145: Appending assistant message
Correctly uses functional updates to maintain immutability.
155-155: Setting loadingSubmit
Properly resets submission state on success.
194-194: Using handleChatResponse
Skips chat creation flow for existing chats. Looks correct.
206-206: Callback for stopping
Proper approach to manage a stop action via state.
219-219: Exposing startChatStream
Allows external control. Implementation is consistent.
221-221: Hook closure
No changes needed.backend/src/guard/chat.guard.ts (7)
6-6: Importing ContextType
Used for distinguishing GraphQL vs. HTTP requests.
25-39: Chat ID requirement
A prior review comment mentioned that chat ID might not be strictly required. Please confirm whether it is truly needed for the stream endpoint.
40-53: GraphQL context
Excluding chat validation forcreateChatmatches your use case. This is straightforward.
57-58: User validation
Throws an exception if not found. Proper check.
61-64: Skipping chat validation
Logical approach for operations that do not require a chat.
66-66: Ownership check comment
This clarifying comment is helpful.
105-107: Reading chatId from WS payload
Sufficient approach for real-time chat subscription.
| this.logger.debug( | ||
| `HTTP Request Headers: ${JSON.stringify(request.headers)}`, | ||
| ); |
There was a problem hiding this comment.
Avoid logging tokens and authorization headers in plain text
Logging entire tokens places them at risk of exposure in log streams or monitoring tools. Similarly, logging the full authorization header can inadvertently reveal sensitive data. Mask or omit these values before writing logs.
Example fix for lines referencing the token and headers:
- this.logger.debug(`HTTP Request Headers: ${JSON.stringify(request.headers)}`);
- this.logger.debug(`Extracted Token: ${token}`);
- this.logger.debug(`Verifying Token: ${token}`);
- this.logger.debug(`Authorization Header: ${authHeader}`);
+ this.logger.debug('HTTP Request Headers: [REDACTED]');
+ this.logger.debug('Extracted Token: [MASKED]');
+ this.logger.debug('Verifying Token: [MASKED]');
+ this.logger.debug('Authorization Header: [MASKED]');Also applies to: 43-44, 74-74, 77-77, 84-84, 89-89, 98-98
…T auth is require
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/src/guard/chat.guard.ts (2)
30-30: Consider simplifying the context type comparison.The type assertion
('graphql' as ContextType)is unnecessarily complex. Since NestJS'scontext.getType()returns a string that can be directly compared, you can simply use:- } else if (contextType === ('graphql' as ContextType)) { + } else if (contextType === 'graphql') {This maintains the same functionality while improving readability.
83-83: Consider making token extraction more robust.The current token extraction assumes a specific format (Bearer token) and might be fragile with optional chaining. Consider adding explicit error handling or format verification:
- const token = client.handshake?.auth?.token?.split(' ')[1]; + const authHeader = client.handshake?.auth?.token; + if (!authHeader || !authHeader.startsWith('Bearer ')) { + throw new UnauthorizedException('Invalid authorization format'); + } + const token = authHeader.split(' ')[1];This provides clearer error messages when the token format is incorrect.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/guard/chat.guard.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Install and Build Frontend
🔇 Additional comments (6)
backend/src/guard/chat.guard.ts (6)
38-39: LGTM: Good handling of various chatId sources.The code correctly handles multiple potential locations for the chatId parameter across different GraphQL operations, making the guard flexible and reusable.
40-44: LGTM: Appropriate special case handling for chat creation.Correctly bypassing chatId validation for the createChat operation makes sense, as a new chat won't have an ID yet.
47-50: LGTM: Common validation logic improves code maintainability.Extracting the user validation into a common section that works for both REST and GraphQL requests is a good practice that reduces duplication.
96-97: LGTM: Clean WebSocket data extraction.The WebSocket data extraction is concise and uses proper destructuring.
25-45: Great multi-context support implementation.The refactored guard effectively handles both REST and GraphQL contexts, making it more versatile while maintaining security. This implementation aligns well with the PR's objective to add REST API support while preserving existing GraphQL functionality.
52-55: LGTM: Addresses previous review feedback.This implementation properly makes chatId optional in certain cases, addressing Sma1lboy's previous comment about only requiring JWT authentication in some scenarios.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor