feat(web): add interactive Excel/Markdown editor components for chat messages#33358
feat(web): add interactive Excel/Markdown editor components for chat messages#33358stonehill-2345 wants to merge 2 commits intolanggenius:mainfrom
Conversation
feat(web): add interactive Excel/Markdown editor components for chat messages See merge request testdev_pub/testify!1
github-actions
bot
added
needs-revision
dosubot
bot
added
size:XXL
Summary of ChangesHello, 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 chat interface by introducing interactive Excel-like spreadsheet and Markdown editor components. These additions transform AI-generated content into dynamic, editable blocks, facilitating human intervention and adjustment within data processing workflows. The changes aim to streamline complex tasks by allowing users to validate, refine, and submit data directly within the conversation, moving away from a tedious 'generate-export-modify-resubmit' cycle to a more integrated and collaborative experience. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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 introduces interactive Excel and Markdown editor components, significantly enhancing the chat message capabilities. The implementation leverages shared hooks for common functionalities like fullscreen toggling, data persistence, and action handling, which promotes code reusability and maintainability. The addition of context menus for the Excel viewer and comprehensive internationalization support are also notable improvements. However, the custom JSON parsing logic for streaming data in both ExcelViewer and MarkdownEditor components appears to be complex and potentially brittle. While it attempts to handle incomplete JSON, relying on string manipulation and regular expressions for this task can lead to unexpected behavior or parsing errors with varied or malformed input. It is recommended to explore more robust JSON streaming parsers or to thoroughly test these parsing mechanisms against a wide range of edge cases to ensure stability and correctness.
I am having trouble creating individual review comments. Click here to see my feedback.
web/app/components/base/markdown-blocks/markdown-editor.tsx (242-258)
The fixJsonNewlines function attempts to correct malformed JSON by replacing unescaped newlines within string values using a regular expression. Parsing JSON with regex is generally unreliable and prone to edge cases, as JSON grammar is more complex than what simple regex can handle robustly (e.g., nested quotes, escaped quotes). This approach can lead to incorrect parsing or missed errors. It would be more robust to ensure the JSON is correctly formatted at the source or to use a JSON parser that offers better error recovery or streaming capabilities.
web/app/components/base/markdown-blocks/excel-viewer.tsx (304-356)
The extractCompleteRows function attempts to manually parse partial JSON arrays from a string. This approach is highly complex and prone to errors, especially when dealing with streaming data where JSON can be truncated at any point. Manually tracking bracket counts and string states to parse JSON is a common source of bugs and can be brittle to unexpected characters or slightly malformed JSON. A more robust JSON streaming parser library or a more thoroughly tested custom implementation that handles all JSON edge cases (e.g., escaped quotes within strings, nested arrays/objects) would significantly improve reliability.
web/app/components/base/markdown-blocks/excel-viewer.tsx (368-377)
The logic for trimming jsonContent based on lastBraceIndex is also quite fragile. If the streamed content contains extraneous characters or is malformed in a way that a } or ] appears prematurely, it could lead to incorrect JSON being parsed or valid content being truncated. This manual string manipulation for JSON parsing is a high-risk area for runtime errors.
web/app/components/base/markdown-blocks/markdown-editor.tsx (271-307)
Similar to ExcelViewer, the parseMarkdownConfig function relies on manual string manipulation and regular expressions (contentMatch, fallbackMatch) to extract content from potentially incomplete JSON. This method is fragile and highly susceptible to variations in the input format or unexpected streaming behavior, which can lead to parsing errors or incorrect content extraction. A more robust JSON parsing strategy, possibly involving a dedicated streaming JSON parser, is recommended to ensure reliability.
web/app/components/base/markdown-blocks/excel-viewer.tsx (672-691)
The handleDataChange logic for merging newDisplayData back into the original data while preserving hidden columns is quite intricate. This manual merging process is complex and could be a source of subtle bugs if not thoroughly tested, especially with operations like adding/deleting rows or columns, or if the hiddenColumnIndices change dynamically. Ensure comprehensive test cases cover various scenarios of editing and hidden columns.
web/app/components/base/markdown-blocks/excel-viewer.tsx (593-599)
The readRemoteFileRef.current(cfg.url, ...) pattern is used here. Since readRemoteFile is a stable function provided by usePapaParse, wrapping it in a useRef and updating the ref in useEffect is not strictly necessary. You can directly call readRemoteFile(cfg.url, ...) within the loadData callback, as readRemoteFile itself is stable across renders.
readRemoteFile(cfg.url, {
download: true,
complete: (results) => {
const rawData = results.data as string[][]
dispatch({ type: 'LOAD_SUCCESS', data: toSpreadsheetData(rawData) })
hasLoadedPersistedDataRef.current = true
},
web/app/components/base/markdown-blocks/excel-viewer.tsx (1087-1102)
The (() => { ... })() pattern for rendering icons based on action?.buttonIcon is verbose. For better readability and maintainability, consider creating a small helper component or a map that directly associates buttonIcon strings with their corresponding RiIcon components. This would make the rendering logic cleaner and easier to extend if more icon types are added.
// Render appropriate icon based on buttonIcon config
{
const IconComponent = {
submit: RiSaveLine,
send: RiSaveLine,
upload: RiSaveLine,
confirm: RiSaveLine,
save: RiSaveLine,
}[action?.buttonIcon || 'save'] || RiSaveLine;
return <IconComponent className="h-4 w-4" />;
}
web/app/components/base/markdown-blocks/hooks/use-editor-common.ts (187-203)
The use of setTimeout with arbitrary delays (100 * (index + 1) and 100 * index) to sequence sendMessage and showToast actions is a workaround. While it ensures a basic order, it's not a robust solution for truly asynchronous or dependent actions. If the post-actions need to be strictly ordered or depend on the completion of previous actions, a more explicit asynchronous flow (e.g., using async/await with a queue or Promise.all for parallel actions) would be more reliable and easier to reason about.
web/app/components/base/markdown-blocks/excel-viewer.tsx (840-875)
The handleContextMenu function relies heavily on specific DOM structure and class names (th, closest('table'), parentElement, children) of the react-spreadsheet library to identify if a row or column header was clicked. This tight coupling to the internal DOM of a third-party library makes the component brittle. Future updates to react-spreadsheet could change its DOM structure, potentially breaking this context menu functionality. It would be more robust to use react-spreadsheet's own APIs for cell/header interaction if they provide such capabilities, or to abstract the DOM querying into a utility that can be easily updated.
web/app/components/base/context-menu/index.tsx (43)
Using key={index} for list items is generally discouraged if the list can change order, be filtered, or have items added/removed in the middle. While the items here are dynamically generated based on context, if the items array itself can be reordered or modified in a way that index no longer uniquely identifies an item, it could lead to unexpected UI behavior or performance issues. Consider using a stable, unique identifier for each ContextMenuItem if available, or generating one if the items are truly transient and their content is unique.
web/eslint-suppressions.json (1652-1654)
The removal of the style/multiline-ternary suppression for agent-content.tsx is a positive change, indicating that the code has been refactored to adhere to the style guide. This improves code readability and consistency.
web/eslint-suppressions.json (2334-2337)
Adding a suppression for ts/no-explicit-any in use-editor-common.ts indicates that there are still areas where type safety could be improved. While sometimes necessary for complex third-party integrations or dynamic data, any types reduce the benefits of TypeScript. Consider refining the types in ActionConfig<T = any> and related functions to be more specific, or adding comments explaining why any is necessary in these specific instances.
web/eslint-suppressions.json (2344-2347)
Adding a suppression for tailwindcss/enforce-consistent-class-order in markdown-editor.tsx suggests a deviation from the configured Tailwind CSS class ordering. Maintaining a consistent class order improves readability and reduces merge conflicts. It's advisable to reorder the classes to conform to the linting rule or to understand why this specific suppression is needed and if it can be avoided.
2 participants
Summary
JSON, supports inline data / URL loading, CSV upload/download, fullscreen,
hidden columns, row/column context menu, and multi-action buttons
JSON, supports inline content / URL loading, live preview, fullscreen, and
multi-action buttons
post-actions) and use-csv-operations (CSV parse/format/upload/download)
renderer and pass messageId for data persistence
Screenshots
Checklist
make lintandmake type-check(backend) andcd web && npx lint-staged(frontend) to appease the lint gods