feat(server,stage-ui): sync message to server#990
Conversation
…pdates - Added chat service to handle chat synchronization and member management. - Introduced chat routes for syncing chat data with the server. - Updated database schema to include new fields for chat members and messages. - Enhanced chat session management in the UI to support new chat features.
⏳ Approval required for deploying to Cloudflare Workers (Preview) for stage-web.
Hey, @nekomeowww, @sumimakito, @luoling8192, @LemonNekoGH, kindly take some time to review and approve this deployment when you are available. Thank you! 🙏 |
Summary of ChangesHello @luoling8192, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new feature to synchronize chat messages and chat session metadata between the client and the server. It involves significant database schema changes to support richer chat functionalities, the creation of a dedicated API endpoint for handling synchronization requests, and the implementation of robust client-side logic to manage and transmit chat data, ensuring data consistency and enabling more dynamic chat interactions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.gh.lumenfield.work/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces chat synchronization functionality, allowing chat sessions and messages to be synced between the client and the server. The changes include schema updates for chat-related tables, a new API endpoint for syncing, and corresponding service logic on the server. On the client side, the chat-session store has been updated to prepare and send chat data to the server using a local-first approach. The implementation generally looks solid, with good use of valibot for schema validation and drizzle-orm for database interactions. The client-side queuing for persistence and synchronization is also well-designed. One comment has been updated to reference a relevant rule regarding programmatic identifiers for event sources.
| ALTER TABLE "chat_members" ADD COLUMN "member_type" text NOT NULL;--> statement-breakpoint | ||
| ALTER TABLE "chat_members" ADD COLUMN "character_id" text;--> statement-breakpoint | ||
| ALTER TABLE "chats" ADD COLUMN "title" text;--> statement-breakpoint | ||
| ALTER TABLE "messages" ADD COLUMN "role" text NOT NULL;--> statement-breakpoint |
There was a problem hiding this comment.
Adding a NOT NULL column role to the messages table without a default value can cause issues if there are existing rows in the messages table. If this migration is applied to an existing database with data, it will fail unless a default value is provided or a two-step migration is performed (add column nullable, update existing rows, then alter column to not null). If this is for a fresh database, it's fine.
| @@ -0,0 +1,6 @@ | |||
| ALTER TABLE "chat_members" ALTER COLUMN "user_id" DROP NOT NULL;--> statement-breakpoint | |||
There was a problem hiding this comment.
The change to user_id from NOT NULL to nullable is a significant schema alteration. While this is likely intended to allow character_id to be present instead, it's important to ensure that the application logic correctly handles cases where user_id might be null, especially if member_type dictates whether user_id or character_id should be present. This could impact data integrity if not properly managed at the application level.
| function resolveSenderId(role: MessageRole, userId: string, characterId?: string) { | ||
| if (role === 'user') | ||
| return userId | ||
| return characterId ?? role |
There was a problem hiding this comment.
The resolveSenderId function defaults to characterId ?? role when the role is not 'user'. If characterId is null, the senderId will be set to the role string itself (e.g., 'system', 'assistant', 'tool', 'error'). While this might be intended, it means these role strings will be stored in the senderId column, which is typically expected to hold actual user or character IDs. Consider if these role strings should be explicitly handled or if a dedicated system_id or similar mechanism is more appropriate to avoid potential confusion or conflicts with actual user/character IDs in the future.
References
- For extensibility, identify the source of an event using a programmatic identifier (e.g., a
sourcefield in metadata) rather than relying on string patterns within the message content.
| mediaIds: [], | ||
| stickerIds: [], |
There was a problem hiding this comment.
The mediaIds and stickerIds arrays are hardcoded to empty arrays when inserting new messages. If the intention is to support media and stickers in messages, the SyncChatMessagePayload interface and the client-side buildSyncMessages function should be updated to include these fields, and the server-side logic should process them accordingly. If they are not currently supported, consider adding a comment to clarify this limitation or plan for future implementation.
| content, | ||
| } satisfies SystemMessage | ||
| createdAt: Date.now(), | ||
| } satisfies ChatHistoryItem |
There was a problem hiding this comment.
The createdAt: Date.now() is added to the initial system message. This is a good change for consistency with other messages and for generating unique message IDs. However, Date.now() provides milliseconds since epoch. Ensure that the server-side timestamp column in Drizzle is configured to handle this precision, or convert it to seconds if the database expects that.
1 participant
No description provided.