feat(ai): Display spinner while creating diff #9991
Conversation
|
miya
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds a progress indicator by tracking when a diff is being applied in the Editor Assistant hook.
- Introduces
isApplyingDiffRefto track diff application state. - Toggles the ref in SSE handlers to mark start/end of diff processing.
Comments suppressed due to low confidence (1)
apps/app/src/features/openai/client/services/editor-assistant.tsx:217
- [nitpick] The name
isApplyingDiffRefcouples implementation detail (Ref) into the variable name. Consider renaming toisApplyingDiff(and using state) for clearer intent.
isApplyingDiffRef.current = true;
| // Refs | ||
| // const positionRef = useRef<number>(0); | ||
| const lineRef = useRef<number>(0); | ||
| const isApplyingDiffRef = useRef<boolean>(false); |
There was a problem hiding this comment.
isApplyingDiffRef isn't returned or used to trigger re-renders in the UI. Consider using useState and returning the boolean so the spinner can actually subscribe to this state.
| const isApplyingDiffRef = useRef<boolean>(false); | |
| const [isApplyingDiff, setIsApplyingDiff] = useState<boolean>(false); |
| const processMessage: ProcessMessage = useCallback((data, handler) => { | ||
| handleIfSuccessfullyParsed(data, SseMessageSchema, (data: SseMessage) => { | ||
| handler.onMessage(data); | ||
| isApplyingDiffRef.current = false; |
There was a problem hiding this comment.
Resetting the ref immediately after handler.onMessage may hide the spinner before the diff has actually been applied. You might want to move this reset to after the diff application completes.
| isApplyingDiffRef.current = false; |
| timerRef.current = setTimeout(() => { | ||
| setIsGeneratingEditorText(true); | ||
| }, 500); | ||
| }; |
There was a problem hiding this comment.
stream でデータを受信してから 500ms の間に次のデータが来ない時にスピナーを表示する
There was a problem hiding this comment.
Pull Request Overview
This PR adds a progress spinner for the editor assistant while generating diffs by introducing a new isGeneratingEditorText state in the hook and passing it down to the sidebar UI.
- Added
isGeneratingEditorTextflag touseEditorAssistantand implemented timer-based show/hide logic. - Updated
MessageCardandAiAssistantSidebarto render aLoadingSpinnerwhenisGeneratingEditorTextis true. - Added corresponding translation keys for the spinner label in all locales.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/app/src/features/openai/client/services/editor-assistant.tsx | Introduced isGeneratingEditorText, timer logic in processMessage, and a new ref for handling spinner timing. |
| apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/MessageCard.tsx | Extended MessageCard API to accept isGeneratingEditorText and render the spinner UI. |
| apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/AiAssistantSidebar.tsx | Passed isGeneratingEditorText prop to MessageCard in the sidebar. |
| apps/app/public/static/locales/*/translation.json | Added text_generation_by_editor_assistant_label entries to all locale files. |
Comments suppressed due to low confidence (2)
apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/MessageCard.tsx:71
- The new spinner UI controlled by
isGeneratingEditorTextis untested. Consider adding a unit or integration test to verify the spinner shows and hides correctly based on the state.
{isGeneratingEditorText && (
apps/app/src/features/openai/client/services/editor-assistant.tsx:73
- [nitpick] The return type for
useEditorAssistanthas been extended withisGeneratingEditorText. Consider updating any accompanying JSDoc or README docs to describe this new property.
type UseEditorAssistant = () => {
| // Refs\ | ||
| const lineRef = useRef<number>(0); | ||
| const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
|
|
There was a problem hiding this comment.
The timeout stored in timerRef isn’t cleared on component unmount, which may lead to memory leaks or unexpected calls. Consider adding a useEffect cleanup to clearTimeout(timerRef.current) when the component unmounts.
| useEffect(() => { | |
| return () => { | |
| if (timerRef.current !== null) { | |
| clearTimeout(timerRef.current); | |
| timerRef.current = null; | |
| } | |
| }; | |
| }, []); |
| } | ||
|
|
||
| // Hide spinner since data is flowing | ||
| setIsGeneratingEditorText(false); |
There was a problem hiding this comment.
Calling setIsGeneratingEditorText(false) on every incoming data chunk may trigger redundant renders. You can add a guard to only update the state when its value actually changes.
| setIsGeneratingEditorText(false); | |
| if (isGeneratingEditorText) { | |
| setIsGeneratingEditorText(false); | |
| } |
| <LoadingSpinner /> | ||
| <span className="ms-2">{t('sidebar_ai_assistant.text_generation_by_editor_assistant_label')}</span> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
下の方の <span className="text-thinking"> と、少なくともスタイルだけでも揃えるべきかな
There was a problem hiding this comment.
text-thinking を適用し、スピナーを消しました (text-thinking 自体がスピナー的な役割をしていると判断したため)
| role: MessageCardRole, | ||
| children: string, | ||
| showActionButtons?: boolean, | ||
| isGeneratingEditorText?: boolean, |
There was a problem hiding this comment.
boolean じゃなくて
<div className="text-muted mb-3">
<LoadingSpinner />
<span className="ms-2">{t('sidebar_ai_assistant.text_generation_by_editor_assistant_label')}</span>
</div>
を丸っと外から入れる方が綺麗だね (editor-assistant.tsx 内で定義したものを props を通じてここまで持ってくる)
そういう意味では action buttons もエディターアシスタント特有のものなのでそうすべきかもしれないが
There was a problem hiding this comment.
MessageCard 側にはエディターアシスタント特有の要素は削除し、additionaltem という Props を設け外からコンポーネントを渡せるようにした。
エディター書き込み時のラベルコンポーネントと、既存の ActionButtons を EditorAssistant の Hook から提供し、AiAssistantSidebar 経由で MessageCard の additionaltem に渡すように改修した。
commit: 73c4b40
| // Refs\ | ||
| const lineRef = useRef<number>(0); | ||
| const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
|
|
| } | ||
|
|
||
| // Hide spinner since data is flowing | ||
| setIsGeneratingEditorText(false); |
Co-authored-by: Copilot <[email protected]>
…message handling and improve code clarity
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a spinner to indicate progress while generating a diff during the AI assistant process. Key changes include the removal or deprecation of the generateMessageCard functionality in the knowledge assistant service, the addition of spinner state management and timer logic in the editor assistant service, and updates to the UI and translation files to support the new spinner indicator.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| apps/app/src/features/openai/client/services/knowledge-assistant.tsx | Removed the generateMessageCard logic and related interfaces, and added a dependency update in the fetch logs effect. |
| apps/app/src/features/openai/client/services/editor-assistant.tsx | Introduced spinner state and timer logic, and refactored action button generation for the editor assistant. |
| apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/MessageCard.tsx | Removed redundant state and callbacks, and introduced a new prop for additional content. |
| apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/AiAssistantSidebar.tsx | Adjusted MessageCard usage to support the new additional content prop, and refactored related variables. |
| Translation files (zh_CN, ja_JP, fr_FR, en_US) | Added spinner label translations for the editor assistant. |
Comments suppressed due to low confidence (2)
apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/MessageCard.tsx:37
- [nitpick] The prop name 'additionaltem' is ambiguous; consider renaming it to 'additionalItem' or 'extraContent' to provide better clarity.
additionaltem,
apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/AiAssistantSidebar.tsx:361
- [nitpick] The variable name 'messageCardAdditionaltemForGeneratingMessage' is unclear; consider renaming it to a more descriptive name such as 'messageCardExtraContentForGenerating' to maintain consistency with other naming conventions.
const messageCardAdditionaltemForGeneratingMessage = useMemo(() => {
| // Views | ||
| initialView: JSX.Element | ||
| generateMessageCard: GenerateMessageCard | ||
| // generateMessageCard: GenerateMessageCard |
There was a problem hiding this comment.
If the generateMessageCard functionality is no longer in use, consider removing the commented-out code entirely to improve clarity and maintainability.
| // generateMessageCard: GenerateMessageCard |
There was a problem hiding this comment.
Pull Request Overview
Adds a delayed spinner indicator in the editor assistant while diff chunks are streaming, refactors message rendering to use a new prop instead of in-card action button logic, and updates translations and cleanup in the knowledge assistant.
- Introduce
isGeneratingEditorTextstate and timer logic inuseEditorAssistantto show a spinner after 500 ms of no data. - Refactor
MessageCardto accept anadditionaltemelement and updateAiAssistantSidebarto render action buttons and the spinner label via that prop. - Remove unused
generateMessageCardhelpers in the knowledge assistant, adjust effect dependencies, and add new translation keys.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/app/src/features/openai/client/services/knowledge-assistant.tsx | Remove unused generateMessageCard, update SWR effect dependencies |
| apps/app/src/features/openai/client/services/editor-assistant.tsx | Add timer/spinner logic, state & callbacks for editor assistant, refactor views |
| apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/MessageCard.tsx | Replace in-card action buttons with additionaltem prop, remove unused state & callbacks |
| apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/AiAssistantSidebar.tsx | Wire up new additionaltem prop, remove commented code |
| apps/app/public/static/locales/*/translation.json | Add text_generation_by_editor_assistant_label key in all supported locales |
Comments suppressed due to low confidence (3)
apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/MessageCard.tsx:40
- [nitpick] The prop name "additionaltem" appears misspelled and unclear; consider renaming it to "additionalItem" or a more descriptive name.
additionaltem?: JSX.Element,
apps/app/src/features/openai/client/services/editor-assistant.tsx:148
- Introduced spinner logic for text generation, but there are no tests verifying the spinner appears after a delay and hides during streaming; consider adding unit or integration tests.
const [isGeneratingEditorText, setIsGeneratingEditorText] = useState<boolean>(false);
apps/app/src/features/openai/client/services/knowledge-assistant.tsx:315
- [nitpick] Adding "aiAssistantSidebarData?.isEditorAssistant" to the fetch-and-set-logs effect dependencies may trigger redundant refetches when toggling the editor assistant UI; consider removing it unless intentional.
}, [threadId, mutateMessageData, setMessageLogs, aiAssistantSidebarData?.isEditorAssistant]);
| handler.onFinalized(data); | ||
| }); | ||
| }, [mutateIsEnableUnifiedMergeView]); | ||
| }, [isGeneratingEditorText, mutateIsEnableUnifiedMergeView]); |
There was a problem hiding this comment.
Including "isGeneratingEditorText" in processMessage's dependency array causes the callback to be recreated on every spinner toggle, which may trigger unnecessary re-subscriptions or re-renders. Consider removing it to stabilize the callback.
| // Views | ||
| initialView: initialViewForKnowledgeAssistant, | ||
| generateMessageCard: generateMessageCardForKnowledgeAssistant, | ||
| // generateMessageCard: generateMessageCardForKnowledgeAssistant, |
There was a problem hiding this comment.
[nitpick] There are multiple commented-out references to generateMessageCard that should be removed to clean up dead code.
| // generateMessageCard: generateMessageCardForKnowledgeAssistant, | |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a delayed spinner indicator during diff/text generation and refactors the message card to support additional embedded items (e.g., buttons or spinner).
- Added
isGeneratingEditorTextstate, timer logic, andgeneratingEditorTextLabelinuseEditorAssistant. - Refactored
MessageCardto take anadditionalItemprop instead of inline action buttons. - Updated
AiAssistantSidebarto inject action buttons and spinner viaadditionalItem.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/app/src/features/openai/client/services/knowledge-assistant.tsx | Extended SWR effect dependencies to include isEditorAssistant state |
| apps/app/src/features/openai/client/services/editor-assistant.tsx | Introduced spinner state/timer logic and new generateActionButtons |
| apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/MessageCard.tsx | Replaced action-button props with additionalItem prop |
| apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/AiAssistantSidebar.tsx | Updated mapping to supply additionalItem for messages |
| apps/app/public/static/locales/*/translation.json | Added translation key for spinner label in multiple locales |
Comments suppressed due to low confidence (4)
apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/AiAssistantSidebar.tsx:399
- Elements in a list should have a
keyprop. Add akey={message.id}to the fragment or theMessageCard.
{ messageLogs.map(message => (
apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/MessageCard.tsx:1
- The import of
type JSXis unused; you can remove it to clean up unused imports.
import { type JSX } from 'react';
apps/app/src/features/openai/client/services/editor-assistant.tsx:2
- The imported
type FCis not used in this file; consider removing it.
useCallback, useEffect, useState, useRef, useMemo, type FC,
apps/app/src/features/openai/client/services/knowledge-assistant.tsx:315
- The effect doesn't reference
isEditorAssistant, so removing it from the dependency array will avoid unnecessary reruns.
}, [threadId, mutateMessageData, setMessageLogs, aiAssistantSidebarData?.isEditorAssistant]); // Dependencies
There was a problem hiding this comment.
Pull Request Overview
This PR adds a progress spinner for diff creation and refactors message card rendering for the Growi AI Editor Assistant. Key changes include:
- Removal of the generateMessageCard helper function and associated interface in favor of new action button components.
- Integration of spinner logic with timer management in the editor assistant service.
- Updates to locale translations to support the new spinner label.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/app/src/features/openai/client/services/knowledge-assistant.tsx | Removed dead code and updated dependency arrays to reflect new spinner state. |
| apps/app/src/features/openai/client/services/editor-assistant.tsx | Added spinner timeout logic and new state for displaying the spinner. |
| apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/MessageCard.tsx | Refactored message card props, removing action button logic in favor of an additionalItem prop. |
| apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/AiAssistantSidebar.tsx | Updated message card rendering to use the new action buttons mechanism based on spinner state. |
| Locale files | Added translations for the spinner label in multiple locales. |
Comments suppressed due to low confidence (4)
apps/app/src/features/openai/client/services/editor-assistant.tsx:224
- [nitpick] The spinner delay of 500ms is hard-coded. Consider defining this timeout as a named constant to improve clarity and facilitate potential adjustments in the future.
timerRef.current = setTimeout(() => { setIsGeneratingEditorText(true); }, 500);
apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/AiAssistantSidebar.tsx:366
- The callback for generating an additional item relies on messageLogs; ensure that messageLogs is always defined in this context to avoid unintentionally omitting the action buttons when they should be displayed.
if (messageId == null || messageLogs == null) {
apps/app/src/features/openai/client/components/AiAssistant/AiAssistantSidebar/MessageCard.tsx:37
- [nitpick] The renaming of props from action button related properties to 'additionalItem' helps clarify the component's role in rendering extra UI content. Please verify that all intended usages have been updated accordingly.
additionalItem,
apps/app/src/features/openai/client/services/knowledge-assistant.tsx:315
- Including 'aiAssistantSidebarData?.isEditorAssistant' in the dependency array may trigger extra re-renders. Please verify that this change is intentional to correctly tie the diff logs update to the spinner state.
}, [threadId, mutateMessageData, setMessageLogs, aiAssistantSidebarData?.isEditorAssistant]); // Dependencies
Task
Screenrecord
2025-05-29.20.30.10.mov