-
Couldn't load subscription status.
- Fork 12
feat: charts on demand #1333
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: charts on demand #1333
Conversation
Signed-off-by: Efren Lim <elim@linuxfoundation.org>
Signed-off-by: Efren Lim <elim@linuxfoundation.org>
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.
Pull Request Overview
This PR enhances the Data Copilot feature by implementing a chart generation workflow with user opt-in and adding feedback functionality. The changes improve the user experience by allowing users to explicitly request chart visualization rather than generating it automatically.
Key changes:
- Added
isChartRequestedstate flag to track when users request chart generation - Implemented chat response feedback mechanism with
isEndResponseflag to mark the final message in a conversation - Modified the results UI to show a prompt banner for chart generation instead of automatically displaying charts
- Removed unused loading state UI elements from the loading-state component
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/app/components/shared/modules/copilot/types/copilot.types.ts |
Added isEndResponse optional property to AIMessage interface |
frontend/app/components/shared/modules/copilot/store/copilot.store.ts |
Added isChartRequested reactive state and initialization logic |
frontend/app/components/shared/modules/copilot/store/copilot.api.service.ts |
Enhanced stream processing to track and propagate chatResponseId throughout the message lifecycle |
frontend/app/components/shared/modules/copilot/components/shared/loading-state.vue |
Removed unused "Data successfully processed" UI block and isDataReady prop usage |
frontend/app/components/shared/modules/copilot/components/results/results-toggle.vue |
Made results tabs dynamic based on isChartRequested state |
frontend/app/components/shared/modules/copilot/components/results/results-section.vue |
Added chart generation prompt banner with opt-in button and handler logic |
frontend/app/components/shared/modules/copilot/components/copilot-sidebar.vue |
Set isEndResponse flag on the last message when chat response completes |
frontend/app/components/shared/modules/copilot/components/chat-history/copilot-chat-item.vue |
Added conditional feedback component for router-status messages |
frontend/app/components/modules/widget/components/shared/widget-menu-popover.vue |
Added @action-done event handler to close popover after action completion |
Comments suppressed due to low confidence (1)
frontend/app/components/shared/modules/copilot/components/shared/loading-state.vue:39
- The
isDataReadyprop is now unused since the related UI block was removed. This prop should be removed from the component's interface to keep the API clean.
isDataReady?: boolean;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <lfx-copilot-results-header | ||
| :is-empty="isEmpty" | ||
| :is-loading="isLoading || isChartLoading" | ||
| :is-loading="isLoading" |
Copilot
AI
Oct 29, 2025
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.
The isChartLoading parameter has been removed from the header's :is-loading prop binding, but the local isChartLoading ref at line 120 and its related handlers (handleChartLoading at line 162) are still present. If chart loading state is no longer needed in the header, consider removing the unused isChartLoading local state and its handler, or document why they're retained.
| } | ||
| if (messages.value.length > 0) { | ||
| messages.value[messages.value.length - 1]!.isEndResponse = true; |
Copilot
AI
Oct 29, 2025
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.
[nitpick] Using non-null assertion operator (!) without prior length check could be risky. Although there's a length check in the if condition above, consider using optional chaining for defensive programming: messages.value[messages.value.length - 1].isEndResponse = true or add a null check inside the if block.
| messages.value[messages.value.length - 1]!.isEndResponse = true; | |
| messages.value[messages.value.length - 1]?.isEndResponse = true; |
In this PR
Tickets
IN-801
IN-800