feat(webui): integrate Runtime WebUI chat into AgentScope Studio#122
feat(webui): integrate Runtime WebUI chat into AgentScope Studio#122zhijianma wants to merge 4 commits intoagentscope-ai:mainfrom
Conversation
Summary of ChangesHello @zhijianma, 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 significantly enhances the AgentScope Studio by introducing a fully integrated and configurable Runtime WebUI chat experience. It provides users with a dedicated interface to interact with agents at runtime, complete with extensive customization options for appearance, messaging, and API connectivity. This addition not only expands the platform's capabilities but also improves the overall user experience by centralizing agent interaction within the studio environment. Highlights
Changelog
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.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates the Runtime WebUI chat into AgentScope Studio, adding a new 'Services' section with a configurable chat interface. The changes are extensive, including new dependencies, routing, UI components, and state management for chat options and sessions. My review focuses on improving robustness, type safety, and maintainability. Key feedback includes addressing the risky practice of monkey-patching console.error, improving the session management logic in sessionApi.ts to be more robust, and enhancing type safety in the new React components to prevent potential runtime errors and improve developer experience.
| // Suppress known harmless warnings from third-party libraries (antd-style/emotion, React/antd) | ||
| // 1. emotion `:first-child` SSR warning — irrelevant for this client-side SPA | ||
| // 2. React `flushSync` lifecycle warning — triggered internally by antd components | ||
| const _origConsoleError = console.error; | ||
| console.error = (...args: unknown[]) => { | ||
| const msg = typeof args[0] === 'string' ? args[0] : ''; | ||
| if (msg.includes(':first-child') && msg.includes('potentially unsafe')) { | ||
| return; | ||
| } | ||
| if (msg.includes('flushSync') && msg.includes('lifecycle')) { | ||
| return; | ||
| } | ||
| _origConsoleError.apply(console, args); | ||
| }; |
There was a problem hiding this comment.
Monkey-patching console.error to suppress warnings is a risky practice. While the suppressed warnings might seem harmless now, this approach can hide other legitimate errors in the future, making debugging difficult. It also affects the entire application globally. Since you've already changed :first-child to :first-of-type in some components, which is the correct fix for the emotion SSR warning, could this global override be removed or scoped more narrowly? If suppression is absolutely necessary, consider creating a higher-order component or a custom hook that temporarily suppresses warnings only around the specific components that trigger them.
| const [formData, setFormData] = useState<Record<string, unknown>>(() => | ||
| structuredClone(value ?? {}), | ||
| ); |
There was a problem hiding this comment.
The component state formData is typed as Record<string, unknown>, which provides very little type safety. This leads to the use of unsafe type assertions (e.g., as string) and risky patterns throughout the component when accessing nested properties. If the data structure changes, these assertions can lead to runtime errors that are hard to debug.
It would be much safer to define a proper interface for the options object and use it for the state: useState<MyOptionsType>(...). This would enable static type checking and improve the overall robustness and maintainability of the component.
| constructor() { | ||
| this.lsKey = 'as-studio-runtime-chat-sessions'; | ||
| this.sessionList = []; | ||
| } |
There was a problem hiding this comment.
The sessionList is only populated from localStorage inside getSessionList, which makes the class fragile as it depends on methods being called in a specific order. Also, JSON.parse in getSessionList is not in a try-catch block, which can crash the app.
Consider loading the data safely in the constructor to ensure the class is always in a valid state upon instantiation. If you apply this change, you can simplify getSessionList to just return [...this.sessionList];.
constructor() {
this.lsKey = 'as-studio-runtime-chat-sessions';
try {
this.sessionList = JSON.parse(localStorage.getItem(this.lsKey) || '[]');
} catch {
this.sessionList = [];
}
}| ); | ||
| } | ||
|
|
||
| const OptionsEditor: React.FC<OptionsEditorProps> = ({ |
There was a problem hiding this comment.
This component is over 400 lines long and handles the editing of many different sections of the configuration (Theme, Sender, Welcome, API). This makes it difficult to read, understand, and maintain.
Consider refactoring this into smaller, more focused components. For example, you could create separate components for each configuration section (ThemeEditor, SenderEditor, etc.). Each of these smaller components would be responsible for its own part of the form, making the code more modular and easier to manage.
| const leftHeaderConfig = ( | ||
| optionsConfig as Record<string, Record<string, unknown>> | ||
| )?.theme?.leftHeader as { logo?: string; title?: string } | undefined; |
There was a problem hiding this comment.
This complex type assertion reduces type safety and makes the code harder to read. It would be better to define a strong type for the optionsConfig state, which would eliminate the need for such casts and improve maintainability. The as unknown as IAgentScopeRuntimeWebUIOptions on line 99 is another symptom of this issue. A stronger type for optionsConfig would allow for safer property access and remove the need for these unsafe casts.
| } | ||
|
|
||
| async createSession(session: Partial<IAgentScopeRuntimeWebUISession>) { | ||
| session.id = Date.now().toString(); |
There was a problem hiding this comment.
Using Date.now().toString() as a session ID is not guaranteed to be unique. If createSession is called multiple times in the same millisecond, it will generate duplicate IDs, which can lead to bugs. A more robust approach is to use a universally unique identifier, like crypto.randomUUID().
session.id = crypto.randomUUID();
|
This PR is marked as stale because there has been no activity for 30 days. Remove stale label or add new comments or this PR will be closed in 5 day. |
1 participant
Description
[Please describe the background, purpose, changes made, and how to test this PR]
Checklist
Please check the following items before code is ready to be reviewed.
npm run formatcommand in the root directory