-
Notifications
You must be signed in to change notification settings - Fork 604
feat: artifactViewMode — opt-in auto-open for artifact detail panels #892
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?
Changes from all commits
016d0df
4dd3c4c
12758b7
00a8164
a4ec353
bdc7a5b
834d0a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { shouldAutoOpen } from "../useArtifactAutoOpen"; | ||
|
|
||
| describe("shouldAutoOpen", () => { | ||
| it.each([ | ||
| ["open-on-mount", true, true], | ||
| ["open-on-mount", false, true], | ||
| ["auto-open", true, true], | ||
| ["auto-open", false, false], | ||
| ["overview", true, false], | ||
| ["overview", false, false], | ||
| ] as const)("mode %s, streaming %s → %s", (mode, isStreaming, expected) => { | ||
| expect(shouldAutoOpen(mode, isStreaming)).toBe(expected); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import { useEffect } from "react"; | ||
| import { useArtifactViewMode, type ArtifactViewMode } from "../store/ArtifactViewModeContext"; | ||
| import { useDetailedViewStore } from "../store/DetailedViewContext"; | ||
|
|
||
| export function shouldAutoOpen(mode: ArtifactViewMode, isStreaming: boolean): boolean { | ||
| return mode === "open-on-mount" || (mode === "auto-open" && isStreaming); | ||
| } | ||
|
|
||
| export interface UseArtifactAutoOpenOptions { | ||
| viewId: string; | ||
| latchKey: string; | ||
| isStreaming: boolean; | ||
| enabled?: boolean; | ||
| } | ||
|
|
||
| export function useArtifactAutoOpen({ | ||
| viewId, | ||
| latchKey, | ||
| isStreaming, | ||
| enabled = true, | ||
| }: UseArtifactAutoOpenOptions): void { | ||
| const viewMode = useArtifactViewMode(); | ||
| const store = useDetailedViewStore(); | ||
|
|
||
| useEffect(() => { | ||
| if (!enabled || !shouldAutoOpen(viewMode, isStreaming)) return; | ||
| const dv = store.getState(); | ||
| if (!dv._markAutoOpened(latchKey)) return; | ||
| const active = dv.activeDetailedViewId; | ||
| if (active !== null && active !== viewId) return; | ||
| dv.setActiveDetailedView(viewId); | ||
| }, [viewMode, enabled, isStreaming, latchKey, viewId, store]); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import { createContext, useContext } from "react"; | ||
|
|
||
| export type ArtifactViewMode = "auto-open" | "open-on-mount" | "overview"; | ||
|
|
||
| export const DEFAULT_ARTIFACT_VIEW_MODE: ArtifactViewMode = "overview"; | ||
|
|
||
| export const ArtifactViewModeContext = createContext<ArtifactViewMode>(DEFAULT_ARTIFACT_VIEW_MODE); | ||
|
|
||
| export function useArtifactViewMode(): ArtifactViewMode { | ||
| return useContext(ArtifactViewModeContext); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { evaluateRegisteredArtifacts } from "../artifactAutoOpenWatcher"; | ||
| import { createDetailedViewStore } from "../createDetailedViewStore"; | ||
| import type { ArtifactEntry } from "../threadContextTypes"; | ||
|
|
||
| const entry = (id: string, version = 1): ArtifactEntry => ({ | ||
| id, | ||
| version, | ||
| heading: `${id} v${version}`, | ||
| type: "test_artifact", | ||
| }); | ||
|
|
||
| const registry = (...entries: ArtifactEntry[]): Record<string, ArtifactEntry[]> => { | ||
| const out: Record<string, ArtifactEntry[]> = {}; | ||
| for (const e of entries) (out[e.id] ??= []).push(e); | ||
| return out; | ||
| }; | ||
|
|
||
| describe("evaluateRegisteredArtifacts", () => { | ||
| it("auto-open: a newly registered artifact opens while the thread runs", () => { | ||
| const store = createDetailedViewStore(); | ||
| evaluateRegisteredArtifacts("auto-open", registry(entry("art")), true, store); | ||
| expect(store.getState().activeDetailedViewId).toBe("art:1"); | ||
| expect(store.getState()._autoOpenedArtifactKeys.has("art")).toBe(true); | ||
| }); | ||
|
|
||
| it("presents once: a user close sticks across re-registrations", () => { | ||
| const store = createDetailedViewStore(); | ||
| const arts = registry(entry("art")); | ||
| evaluateRegisteredArtifacts("auto-open", arts, true, store); | ||
| store.getState().setActiveDetailedView(null); | ||
| evaluateRegisteredArtifacts("auto-open", arts, true, store); | ||
| expect(store.getState().activeDetailedViewId).toBeNull(); | ||
| }); | ||
|
|
||
| it("edits never re-open: a new version shares the claimed id", () => { | ||
| const store = createDetailedViewStore(); | ||
| evaluateRegisteredArtifacts("auto-open", registry(entry("art", 1)), true, store); | ||
| store.getState().setActiveDetailedView(null); | ||
| evaluateRegisteredArtifacts( | ||
| "auto-open", | ||
| registry(entry("art", 1), entry("art", 2)), | ||
| true, | ||
| store, | ||
| ); | ||
| expect(store.getState().activeDetailedViewId).toBeNull(); | ||
| }); | ||
|
|
||
| it("opens the latest registered version of an id", () => { | ||
| const store = createDetailedViewStore(); | ||
| evaluateRegisteredArtifacts( | ||
| "open-on-mount", | ||
| registry(entry("art", 1), entry("art", 3)), | ||
| false, | ||
| store, | ||
| ); | ||
| expect(store.getState().activeDetailedViewId).toBe("art:3"); | ||
| }); | ||
|
|
||
| it("auto-open: historical registrations (thread not running) never open — and stay claimed", () => { | ||
| const store = createDetailedViewStore(); | ||
| const arts = registry(entry("old")); | ||
| evaluateRegisteredArtifacts("auto-open", arts, false, store); | ||
| expect(store.getState().activeDetailedViewId).toBeNull(); | ||
| evaluateRegisteredArtifacts("auto-open", arts, true, store); | ||
| expect(store.getState().activeDetailedViewId).toBeNull(); | ||
| }); | ||
|
|
||
| it("open-on-mount: opens on thread load with nothing running", () => { | ||
| const store = createDetailedViewStore(); | ||
| evaluateRegisteredArtifacts("open-on-mount", registry(entry("art")), false, store); | ||
| expect(store.getState().activeDetailedViewId).toBe("art:1"); | ||
| }); | ||
|
|
||
| it("first wins: an open panel is never stolen by another artifact", () => { | ||
| const store = createDetailedViewStore(); | ||
| evaluateRegisteredArtifacts("auto-open", registry(entry("a1"), entry("a2")), true, store); | ||
| expect(store.getState().activeDetailedViewId).toBe("a1:1"); | ||
| expect(store.getState()._autoOpenedArtifactKeys.has("a2")).toBe(true); | ||
| store.getState().setActiveDetailedView(null); | ||
| evaluateRegisteredArtifacts("auto-open", registry(entry("a1"), entry("a2")), true, store); | ||
| expect(store.getState().activeDetailedViewId).toBeNull(); | ||
| }); | ||
|
|
||
| it("first wins: a user-opened panel blocks auto-open the same way", () => { | ||
| const store = createDetailedViewStore(); | ||
| store.getState().setActiveDetailedView("user-panel"); | ||
| evaluateRegisteredArtifacts("auto-open", registry(entry("art")), true, store); | ||
| expect(store.getState().activeDetailedViewId).toBe("user-panel"); | ||
| expect(store.getState()._autoOpenedArtifactKeys.has("art")).toBe(true); | ||
| }); | ||
|
|
||
| it("overview: never opens and never claims", () => { | ||
| const store = createDetailedViewStore(); | ||
| evaluateRegisteredArtifacts("overview", registry(entry("art")), true, store); | ||
| expect(store.getState().activeDetailedViewId).toBeNull(); | ||
| expect(store.getState()._autoOpenedArtifactKeys.size).toBe(0); | ||
| }); | ||
|
|
||
| it("thread switch (reset) re-arms for the next thread", () => { | ||
| const store = createDetailedViewStore(); | ||
| const arts = registry(entry("art")); | ||
| evaluateRegisteredArtifacts("open-on-mount", arts, false, store); | ||
| expect(store.getState().activeDetailedViewId).toBe("art:1"); | ||
| store.getState().reset(); | ||
| evaluateRegisteredArtifacts("open-on-mount", arts, false, store); | ||
| expect(store.getState().activeDetailedViewId).toBe("art:1"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { createDetailedViewStore } from "../createDetailedViewStore"; | ||
|
|
||
| describe("detailed-view auto-open latch", () => { | ||
| it("claims a key exactly once", () => { | ||
| const store = createDetailedViewStore(); | ||
|
|
||
| expect(store.getState()._markAutoOpened("a1:1")).toBe(true); | ||
| expect(store.getState()._markAutoOpened("a1:1")).toBe(false); | ||
| }); | ||
|
|
||
| it("treats a new version (edit) as a fresh key", () => { | ||
| const store = createDetailedViewStore(); | ||
|
|
||
| expect(store.getState()._markAutoOpened("a1:1")).toBe(true); | ||
| expect(store.getState()._markAutoOpened("a1:2")).toBe(true); | ||
| }); | ||
|
|
||
| it("clears claimed keys on reset (thread switch)", () => { | ||
| const store = createDetailedViewStore(); | ||
|
|
||
| expect(store.getState()._markAutoOpened("a1:1")).toBe(true); | ||
| store.getState().reset(); | ||
| expect(store.getState()._markAutoOpened("a1:1")).toBe(true); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import { useEffect } from "react"; | ||
| import { shouldAutoOpen } from "../hooks/useArtifactAutoOpen"; | ||
| import type { ArtifactViewMode } from "./ArtifactViewModeContext"; | ||
| import type { createChatStore } from "./createChatStore"; | ||
| import type { createDetailedViewStore } from "./createDetailedViewStore"; | ||
| import type { createThreadContextStore } from "./createThreadContextStore"; | ||
| import type { ArtifactEntry } from "./threadContextTypes"; | ||
|
|
||
| export function evaluateRegisteredArtifacts( | ||
| viewMode: ArtifactViewMode, | ||
| artifacts: Record<string, ArtifactEntry[]>, | ||
| isThreadRunning: boolean, | ||
| detailedViewStore: ReturnType<typeof createDetailedViewStore>, | ||
| ): void { | ||
| if (viewMode === "overview") return; | ||
| const mayOpen = shouldAutoOpen(viewMode, isThreadRunning); | ||
|
|
||
| for (const versions of Object.values(artifacts)) { | ||
| const latest = versions[versions.length - 1]; | ||
| if (!latest) continue; | ||
| const dv = detailedViewStore.getState(); | ||
| if (!dv._markAutoOpened(latest.id)) continue; | ||
| if (!mayOpen) continue; | ||
| if (dv.activeDetailedViewId !== null) continue; | ||
| dv.setActiveDetailedView(`${latest.id}:${latest.version}`); | ||
| } | ||
| } | ||
|
|
||
| export function useArtifactAutoOpenWatcher( | ||
| viewMode: ArtifactViewMode, | ||
| chatStore: ReturnType<typeof createChatStore>, | ||
| threadContextStore: ReturnType<typeof createThreadContextStore>, | ||
| detailedViewStore: ReturnType<typeof createDetailedViewStore>, | ||
| ): void { | ||
| useEffect(() => { | ||
| if (viewMode === "overview") return; | ||
| return threadContextStore.subscribe( | ||
| (s) => s.artifacts, | ||
| (artifacts) => { | ||
| evaluateRegisteredArtifacts( | ||
| viewMode, | ||
| artifacts, | ||
| chatStore.getState().isRunning, | ||
| detailedViewStore, | ||
| ); | ||
| }, | ||
| { fireImmediately: true }, | ||
| ); | ||
| }, [viewMode, chatStore, threadContextStore, detailedViewStore]); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,10 @@ export type DetailedViewInternals = { | |
| _detailedViewPanelNode: HTMLElement | null; | ||
| /** @internal */ | ||
| _setDetailedViewPanelNode: (node: HTMLElement | null) => void; | ||
| /** @internal */ | ||
| _autoOpenedArtifactKeys: ReadonlySet<string>; | ||
| /** @internal */ | ||
| _markAutoOpened: (key: string) => boolean; | ||
| }; | ||
|
|
||
| /** Combined detailed-view store type (state + actions + internals). */ | ||
|
Comment on lines
35
to
44
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is happening here and why are we changing this ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the state behind the new |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -183,6 +183,7 @@ export const AgentInterface: AgentInterfaceComponent = ((props: AgentInterfacePr | |
| llm, | ||
| artifactRenderers, | ||
| artifactCategories, | ||
| artifactViewMode, | ||
| componentLibrary, | ||
| components, | ||
| theme, | ||
|
|
@@ -240,6 +241,7 @@ export const AgentInterface: AgentInterfaceComponent = ((props: AgentInterfacePr | |
| llm={llm} | ||
| artifactRenderers={artifactRenderers} | ||
| artifactCategories={artifactCategories} | ||
| artifactViewMode={artifactViewMode} | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks good.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 — and note it's |
||
| > | ||
| <NavProvider path={path} defaultPath={defaultPath} onNavigate={onNavigate}> | ||
| <StartersProvider starters={starters} starterVariant={starterVariant}> | ||
|
|
||
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.
Explain this part?
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.
This is the auto-open latch's check-and-claim, done as one atomic call so there's no gap between "has it fired?" and "mark it fired":
keys.has(key)→ already claimed → returnfalse, and the caller skips opening. This is what makes a user's mid-stream close stick: if the renderer host remounts during streaming (it does — React StrictMode and streaming re-renders both remount it), the fresh instance asks again, getsfalse, and does NOT re-open over the close.new Set(keys).add(key)— a copy, notkeys.add(key)in place, because zustand subscribers compare by reference; mutating the existing Set would be an invisible state change. Copying keeps the store honest.reset()(one line up) clears the Set together with the active view on thread switch, so a fresh thread starts with a clean slate.