Skip to content

feat: Display viz in Artifact panel#16499

Merged
tatianainama merged 1 commit intomainfrom
08-19-feat_display_viz_in_artifact_panel
Aug 22, 2025
Merged

feat: Display viz in Artifact panel#16499
tatianainama merged 1 commit intomainfrom
08-19-feat_display_viz_in_artifact_panel

Conversation

@tatianainama
Copy link
Copy Markdown
Contributor

@tatianainama tatianainama commented Aug 19, 2025

Description:

Using previous work to display visualizations into the Artifact panel

Screen Recording 2025-08-20 at 17.08.54.mov (uploaded via Graphite)

Next

  • Improve animations and interactions

Copy link
Copy Markdown
Contributor Author

tatianainama commented Aug 19, 2025

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 19, 2025

Your preview environment pr-16499 has been deployed with errors.

Preview environment endpoints are available at:

</Paper>
<AiArtifactButton
onClick={() =>
setArtifact(renderArtifact(), message.uuid)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will use artifact uuid once we have the backend in place, and we could move this logic to a new place, where we can make the queries from that component instead of passing everything here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes sounds good!

@tatianainama tatianainama force-pushed the 08-19-feat_introduce_artifact_panel branch from ddd669e to 5ecd1a0 Compare August 20, 2025 15:49
@tatianainama tatianainama force-pushed the 08-19-feat_display_viz_in_artifact_panel branch from 9cdca7a to 627591e Compare August 20, 2025 15:49
@tatianainama tatianainama requested a review from notgiorgi August 20, 2025 16:05
@tatianainama tatianainama changed the base branch from 08-19-feat_introduce_artifact_panel to graphite-base/16499 August 20, 2025 16:38
@tatianainama tatianainama force-pushed the 08-19-feat_display_viz_in_artifact_panel branch from 627591e to afab005 Compare August 21, 2025 07:53
Comment on lines +98 to +102
useEffect(() => {
if (contextArtifact) {
clearArtifact();
}
}, [agentUuid, threadUuid, projectUuid]); // eslint-disable-line react-hooks/exhaustive-deps
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This useEffect hook disables the react-hooks/exhaustive-deps ESLint rule, which violates Frontend rules anti-patterns. According to the Frontend rules, when this disable comment is needed, there is likely a better approach to structuring the data. Instead of using an effect to clear contextArtifact when route parameters change, consider deriving the artifact state based on the current route parameters rather than relying on an effect with disabled dependency warnings.

Spotted by Diamond (based on custom rule: packages/frontend rules)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +398 to +423
useEffect(() => {
// We want to auto-open artifact for messages that just completed streaming and have viz
// in the future, we want to track the viz calls to start showing artifact while streaming
if (
justCompleted &&
isVisualizationAvailable &&
!isQueryError &&
!isQueryLoading &&
queryResults
) {
setArtifact(renderArtifact(), message.uuid);
clearMessageJustCompleted(message.threadUuid);
}
}, [
justCompleted,
isVisualizationAvailable,
isQueryError,
isQueryLoading,
queryResults,
renderArtifact,
setArtifact,
clearMessageJustCompleted,
message.threadUuid,
message.uuid,
]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Graphite also mentioned it in the comment but, I'd like to see if there's a better way to do this instead of useEffect+setState pattern. Or maybe this is temporary before we hook-up with artifacts api?


const setArtifact = (artifact: ReactNode) => {
setContextArtifact(artifact);
const setArtifact = (artifact: ReactNode, id: string) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does setArtifact need to accept artifact: ReactNode as a param? just the id should be enough to decide wether or not artifact is set and then the component would derive rest

Copy link
Copy Markdown
Contributor Author

@tatianainama tatianainama Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, great point! I didn't think of that, let me see what can I improve

Update: Yeah so the issue here is the current component tree:

┌─AiAgentPageLayout  ────────────────────────────────────────────┐
│ (provides context of layout)                                   │
│                                                                │
│ ┌─PanelGroup ────────────────────────────────────────────────┐ │
│ │                                                            │ │
│ │ ┌─Sidebar────┐ ┌─Chat──────────────────┐ ┌ Artifact──────┐ │ │
│ │ │            │ │ ┌─AgentChatDisplay ─┐ │ │               │ │ │
│ │ │            │ │ │                   │ │ │               │ │ │
│ │ │            │ │ │ ┌UserBubble─────┐ │ │ │  render()     │ │ │
│ │ │            │ │ │ │               │ │ │ │  ▲            │ │ │
│ │ │            │ │ │ │               │ │ │ │  │            │ │ │
│ │ │            │ │ │ │               │ │ │ │  │            │ │ │
│ │ │            │ │ │ │               │ │ │ │  │            │ │ │
│ │ │            │ │ │ └───────────────┘ │ │ │  │            │ │ │
│ │ │            │ │ │                   │ │ │  │            │ │ │
│ │ │            │ │ │ ┌AssistantBubble┐ │ │ │  │            │ │ │
│ │ │            │ │ │ │     viz───────┼─┼─┼─┼──┘            │ │ │
│ │ │            │ │ │ │               │ │ │ │               │ │ │
│ │ │            │ │ │ └───────────────┘ │ │ │               │ │ │
│ │ │            │ │ └───────────────────┘ │ │               │ │ │
│ │ └────────────┘ └───────────────────────┘ └───────────────┘ │ │
│ └────────────────────────────────────────────────────────────┘ │
│                                                                │
└────────────────────────────────────────────────────────────────┘

the AssistantBubble has access to all the rendering context (vizConfig, queryResults, message data) but the AiAgentPageLayout needs to render the content in the Artifact panel.

A solution could be introducing React portals, let the assistant bubble render directly into the great-grandparent AiAgentPageLayout.

But we will eventually move to passing only the artifact id, and introduce a new artifact component that can fetch that artifact (and versions, etc) and be isolated form the assistant bubble. I think we could leave this as it is for now and tackle the refactor once we come back to this work? What do you think?

Copy link
Copy Markdown
Contributor Author

@tatianainama tatianainama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slack conversations are not working correctly because of provider missing. Will fix!

Comment on lines 18 to 24
}

export const AiAgentPageLayoutContext =
createContext<AiAgentPageLayoutContextType | null>(null);

export const useAiAgentPageLayout = () => {
const context = useContext(AiAgentPageLayoutContext);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The useAiAgentPageLayout hook should check if the context exists before returning it to prevent null reference errors. Restore the null check to ensure type safety when accessing context properties like 'isSidebarCollapsed'.

Spotted by Diamond (based on CI logs)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Copy link
Copy Markdown
Contributor Author

tatianainama commented Aug 22, 2025

Merge activity

  • Aug 22, 8:58 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Aug 22, 8:58 AM UTC: @tatianainama merged this pull request with Graphite.

@tatianainama tatianainama merged commit b79c53d into main Aug 22, 2025
21 of 23 checks passed
@tatianainama tatianainama deleted the 08-19-feat_display_viz_in_artifact_panel branch August 22, 2025 08:58
lightdash-bot pushed a commit that referenced this pull request Aug 22, 2025
# [0.1944.0](0.1943.1...0.1944.0) (2025-08-22)

### Features

* Display viz in Artifact panel ([#16499](#16499)) ([b79c53d](b79c53d))
@lightdash-bot
Copy link
Copy Markdown
Collaborator

🎉 This PR is included in version 0.1944.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

magnew pushed a commit that referenced this pull request Aug 25, 2025
### Description:
Using previous work to display visualizations into the Artifact panel

[Screen Recording 2025-08-20 at 17.08.54.mov <span class="graphite__hidden">(uploaded via Graphite)</span> <img class="graphite__hidden" src="https://app.graphite.dev/user-attachments/thumbnails/f6ee543d-6e56-4b34-9609-78fe4fc36614.mov" />](https://app.graphite.dev/user-attachments/video/f6ee543d-6e56-4b34-9609-78fe4fc36614.mov)

### Next
- Improve animations and interactions
magnew pushed a commit to Abhijay007/lightdash that referenced this pull request Aug 25, 2025
# [0.1944.0](lightdash/lightdash@0.1943.1...0.1944.0) (2025-08-22)

### Features

* Display viz in Artifact panel ([lightdash#16499](lightdash#16499)) ([b79c53d](lightdash@b79c53d))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants