-
Notifications
You must be signed in to change notification settings - Fork 15
Feat/AI SDK Migration IJ #138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feat/AI SDK Migration IJ #138
Conversation
- Migrate chat functionality from Anthropic SDK to Vercel AI SDK - Add comprehensive test coverage for AI chat hooks - Update .gitignore to exclude test artifacts, build files, and environment files - Add new API routes for chat and prompt-type handling - Update architecture documentation - Remove deprecated conversational.go files - Add new AI SDK implementation files
- Move debugging documentation to docs/debugging/ (ignored) - Move setup documentation to docs/local-setup/ (ignored) - Move debugging scripts to scripts/debugging/ (ignored) - Move setup scripts to scripts/setup/ (ignored) - Update .gitignore to exclude these directories - Remove CLAUDE.md (moved to appropriate location) - Clean up root directory to only essential files These files remain available locally for reference but are not committed to git.
- Update .gitignore to exclude entire docs/ directory - Remove all tracked files in docs/ from git (files remain locally) - All documentation now kept locally only, not committed to repository
| aiChatHook.setSelectedRole("auto"); | ||
| } | ||
| // Cast to HTMLFormElement for the hook's handleSubmit | ||
| aiChatHook.handleSubmit(e as React.FormEvent<HTMLFormElement>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Race condition in role selection during new chart submission
When submitting a new chart, if selectedRole is not "auto", the code calls setSelectedRole("auto") followed immediately by handleSubmit. However, setSelectedRole is a React state setter that schedules an async update. The selectedRoleRef.current used inside the transport's prepareSendMessagesRequest won't reflect the new "auto" value until the next render. The message will be sent with the old role value instead of "auto" as intended.
Additional Locations (1)
| { error: 'Internal server error' }, | ||
| { status: 500 } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Test expects logging but implementation doesn't log errors
The catch block in the chat API route silently swallows network errors without any logging. The corresponding test expects console.error to be called when a network error occurs, but the implementation doesn't log anything. This will cause the test to fail and means network errors in production won't be logged for debugging.
Additional Locations (1)
| ], | ||
| workspaceId: mockWorkspaceId, | ||
| userId: mockUserId, | ||
| }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Test expectation mismatches actual role field in request body
The test for "should proxy request to Go backend with correct format" sends a request with role: 'developer' but expects the proxied body to NOT include the role field. However, the actual implementation includes role: role || 'auto' in the body sent to the Go backend. The test assertion will fail because the implementation includes role: 'developer' in the request body.
Additional Locations (1)
|
|
||
| // If messages are empty, store the plan for later application | ||
| if (prevMessages.length === 0) { | ||
| console.log('Messages empty, storing plan for later:', plan.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Debug console.log statements left in production code
Multiple console.log debug statements were left in the Centrifugo hook. These include logging every received Centrifugo message with event type and full data (line 437), plan-updated events (line 440), current messages count (line 451), and when plans are stored for later (line 455). These appear to be debugging statements that weren't removed before commit.
| await db.query( | ||
| `UPDATE workspace_chat SET response = $1 WHERE id = $2 AND workspace_id = $3`, | ||
| [response, messageId, workspaceId] | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing authorization check allows updating any message
The PATCH endpoint obtains userId from authentication at line 59 but never uses it to verify the user has permission to modify the message. The SQL query only checks that messageId and workspaceId match, allowing any authenticated user to update any message in any workspace if they know or guess the IDs. The query needs to include a check that the user owns the workspace or message.
No description provided.