Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ export const AssistantChat: React.FunctionComponent<AssistantChatProps> = ({
void ensureOptInAndSend?.(undefined, {}, () => {});
}
},
[ensureOptInAndSend, setMessages]
[ensureOptInAndSend, setMessages, track]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

drive-by

);

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ type CompassComponentsProviderProps = {
itemGroup: ContextMenuItemGroup,
item: ContextMenuItem
) => void;
} & {
onDrawerSectionOpen?: (drawerSectionId: string) => void;
onDrawerSectionHide?: (drawerSectionId: string) => void;
} & React.ComponentProps<typeof SignalHooksProvider>;

const darkModeMediaQuery = (() => {
Expand Down Expand Up @@ -119,6 +122,8 @@ export const CompassComponentsProvider = ({
onNextGuideCueGroup,
onContextMenuOpen,
onContextMenuItemClick,
onDrawerSectionOpen,
onDrawerSectionHide,
utmSource,
utmMedium,
stackedElementsZIndex,
Expand Down Expand Up @@ -149,7 +154,10 @@ export const CompassComponentsProvider = ({
darkMode={darkMode}
popoverPortalContainer={popoverPortalContainer}
>
<DrawerContentProvider>
<DrawerContentProvider
onDrawerSectionOpen={onDrawerSectionOpen}
onDrawerSectionHide={onDrawerSectionHide}
>
<StackedComponentProvider zIndex={stackedElementsZIndex}>
<RequiredURLSearchParamsProvider
utmSource={utmSource}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
useDrawerActions,
} from './drawer-portal';
import { expect } from 'chai';
import sinon from 'sinon';

describe('DrawerSection', function () {
it('renders DrawerSection in the portal and updates the content when it updates', async function () {
Expand Down Expand Up @@ -60,8 +61,14 @@ describe('DrawerSection', function () {
const icons = ['ArrowDown', 'CaretDown', 'ChevronDown'] as const;

it('switches drawer content when selecting a different section in the toolbar', async function () {
const onDrawerSectionOpenSpy = sinon.spy();
const onDrawerSectionHideSpy = sinon.spy();

render(
<DrawerContentProvider>
<DrawerContentProvider
onDrawerSectionOpen={onDrawerSectionOpenSpy}
onDrawerSectionHide={onDrawerSectionHideSpy}
>
<DrawerAnchor>
{[1, 2, 3].map((n, idx) => {
return (
Expand All @@ -85,33 +92,62 @@ describe('DrawerSection', function () {
expect(screen.getByText('This is section 1')).to.be.visible;
});

expect(onDrawerSectionOpenSpy).to.have.been.calledOnceWith('section-1');
onDrawerSectionOpenSpy.resetHistory();

userEvent.click(screen.getByRole('button', { name: 'Section 2' }));
await waitFor(() => {
expect(screen.getByText('This is section 2')).to.be.visible;
});

expect(onDrawerSectionHideSpy).to.have.been.calledOnceWith('section-1');
expect(onDrawerSectionOpenSpy).to.have.been.calledOnceWith('section-2');
onDrawerSectionOpenSpy.resetHistory();
onDrawerSectionHideSpy.resetHistory();

userEvent.click(screen.getByRole('button', { name: 'Section 3' }));
await waitFor(() => {
expect(screen.getByText('This is section 3')).to.be.visible;
});

expect(onDrawerSectionHideSpy).to.have.been.calledOnceWith('section-2');
expect(onDrawerSectionOpenSpy).to.have.been.calledOnceWith('section-3');
onDrawerSectionOpenSpy.resetHistory();
onDrawerSectionHideSpy.resetHistory();

userEvent.click(screen.getByRole('button', { name: 'Section 1' }));
await waitFor(() => {
expect(screen.getByText('This is section 1')).to.be.visible;
});

expect(onDrawerSectionHideSpy).to.have.been.calledOnceWith('section-3');
expect(onDrawerSectionOpenSpy).to.have.been.calledOnceWith('section-1');
onDrawerSectionOpenSpy.resetHistory();
onDrawerSectionHideSpy.resetHistory();

userEvent.click(screen.getByRole('button', { name: 'Close drawer' }));
await waitFor(() => {
expect(screen.queryByText('This is section 1')).not.to.exist;
expect(screen.queryByText('This is section 2')).not.to.exist;
expect(screen.queryByText('This is section 3')).not.to.exist;
});

expect(onDrawerSectionHideSpy).to.have.been.calledOnceWith('section-1');
expect(onDrawerSectionOpenSpy).to.not.have.been.called;
onDrawerSectionOpenSpy.resetHistory();
onDrawerSectionHideSpy.resetHistory();
});

it('closes drawer when opened section is removed from toolbar data', async function () {
const onDrawerSectionOpenSpy = sinon.spy();
const onDrawerSectionHideSpy = sinon.spy();

// Render two sections, auto-open first one
const { rerender } = render(
<DrawerContentProvider>
<DrawerContentProvider
onDrawerSectionOpen={onDrawerSectionOpenSpy}
onDrawerSectionHide={onDrawerSectionHideSpy}
>
<DrawerAnchor>
<DrawerSection
id="test-section-1"
Expand All @@ -138,9 +174,19 @@ describe('DrawerSection', function () {
expect(screen.getByText('This is a test section')).to.be.visible;
});

expect(onDrawerSectionHideSpy).to.not.have.been.called;
expect(onDrawerSectionOpenSpy).to.have.been.calledOnceWith(
'test-section-1'
);
onDrawerSectionOpenSpy.resetHistory();
onDrawerSectionHideSpy.resetHistory();

// Now render without opened section
rerender(
<DrawerContentProvider>
<DrawerContentProvider
onDrawerSectionOpen={onDrawerSectionOpenSpy}
onDrawerSectionHide={onDrawerSectionHideSpy}
>
<DrawerAnchor>
<DrawerSection
id="test-section-2"
Expand All @@ -163,9 +209,17 @@ describe('DrawerSection', function () {
// drawer with toolbar
screen.getByTestId('lg-drawer')
).to.have.attribute('aria-hidden', 'true');

expect(onDrawerSectionHideSpy).to.have.been.calledOnceWith(
'test-section-1'
);
expect(onDrawerSectionOpenSpy).to.not.have.been.called;
});

it('can control drawer state via the hooks', async function () {
const onDrawerSectionOpenSpy = sinon.spy();
const onDrawerSectionHideSpy = sinon.spy();

const ControlElement = () => {
const { isDrawerOpen } = useDrawerState();
const { openDrawer, closeDrawer } = useDrawerActions();
Expand All @@ -188,7 +242,10 @@ describe('DrawerSection', function () {
);
};
render(
<DrawerContentProvider>
<DrawerContentProvider
onDrawerSectionOpen={onDrawerSectionOpenSpy}
onDrawerSectionHide={onDrawerSectionHideSpy}
>
<ControlElement />
<DrawerAnchor>
<DrawerSection
Expand All @@ -214,19 +271,36 @@ describe('DrawerSection', function () {
// Drawer is closed by default
expect(screen.getByTestId('drawer-state')).to.have.text('closed');

expect(onDrawerSectionHideSpy).to.not.have.been.called;
expect(onDrawerSectionOpenSpy).to.not.have.been.called;
onDrawerSectionOpenSpy.resetHistory();
onDrawerSectionHideSpy.resetHistory();

// Open the drawer
userEvent.click(screen.getByRole('button', { name: 'Hook Open drawer' }));
await waitFor(() => {
expect(screen.getByTestId('drawer-state')).to.have.text('open');
expect(screen.getByText('This is the controlled section')).to.be.visible;
});

expect(onDrawerSectionHideSpy).to.not.have.been.called;
expect(onDrawerSectionOpenSpy).to.have.been.calledOnceWith(
'controlled-section'
);
onDrawerSectionOpenSpy.resetHistory();
onDrawerSectionHideSpy.resetHistory();

// Close the drawer
userEvent.click(screen.getByRole('button', { name: 'Hook Close drawer' }));
await waitFor(() => {
expect(screen.getByTestId('drawer-state')).to.have.text('closed');
expect(screen.queryByText('This is the controlled section')).not.to.exist;
});

expect(onDrawerSectionHideSpy).to.have.been.calledOnceWith(
'controlled-section'
);
expect(onDrawerSectionOpenSpy).to.not.have.been.called;
});

it('renders guide cue when passed in props', async function () {
Expand Down
65 changes: 59 additions & 6 deletions packages/compass-components/src/components/drawer-portal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ const DrawerOpenStateContext =
const DrawerSetOpenStateContext =
React.createContext<DrawerSetOpenStateContextValue>(() => {});

type DrawerCurrentTabStateContextValue = string | null;

type DrawerSetCurrentTabContextValue = (currentTab: string | null) => void;

const DrawerCurrentTabStateContext =
React.createContext<DrawerCurrentTabStateContextValue>(null);

const DrawerSetCurrentTabContext =
React.createContext<DrawerSetCurrentTabContextValue>(() => {});

const DrawerActionsContext = React.createContext<DrawerActionsContextValue>({
current: {
openDrawer: () => undefined,
Expand Down Expand Up @@ -104,12 +114,16 @@ const DrawerActionsContext = React.createContext<DrawerActionsContextValue>({
* )
* }
*/
export const DrawerContentProvider: React.FunctionComponent = ({
children,
}) => {
export const DrawerContentProvider: React.FunctionComponent<{
onDrawerSectionOpen?: (drawerSectionId: string) => void;
onDrawerSectionHide?: (drawerSectionId: string) => void;
children?: React.ReactNode;
}> = ({ onDrawerSectionOpen, onDrawerSectionHide, children }) => {
const [drawerState, setDrawerState] = useState<DrawerSectionProps[]>([]);
const [drawerOpenState, setDrawerOpenState] =
useState<DrawerOpenStateContextValue>(false);
const [drawerCurrentTab, setDrawerCurrentTab] =
useState<DrawerCurrentTabStateContextValue>(null);
const drawerActions = useRef({
openDrawer: () => undefined,
closeDrawer: () => undefined,
Expand All @@ -135,13 +149,42 @@ export const DrawerContentProvider: React.FunctionComponent = ({
},
});

const prevDrawerCurrentTabRef = React.useRef<string | null>(null);

useEffect(() => {
if (drawerCurrentTab === prevDrawerCurrentTabRef.current) {
// ignore unless it changed
return;
}

if (
drawerCurrentTab &&
drawerCurrentTab !== prevDrawerCurrentTabRef.current
) {
onDrawerSectionOpen?.(drawerCurrentTab);
}

if (
prevDrawerCurrentTabRef.current &&
drawerCurrentTab !== prevDrawerCurrentTabRef.current
) {
onDrawerSectionHide?.(prevDrawerCurrentTabRef.current);
}

prevDrawerCurrentTabRef.current = drawerCurrentTab;
}, [drawerCurrentTab, onDrawerSectionHide, onDrawerSectionOpen]);

return (
<DrawerStateContext.Provider value={drawerState}>
<DrawerOpenStateContext.Provider value={drawerOpenState}>
<DrawerSetOpenStateContext.Provider value={setDrawerOpenState}>
<DrawerActionsContext.Provider value={drawerActions}>
{children}
</DrawerActionsContext.Provider>
<DrawerCurrentTabStateContext.Provider value={drawerCurrentTab}>
<DrawerSetCurrentTabContext.Provider value={setDrawerCurrentTab}>
<DrawerActionsContext.Provider value={drawerActions}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The nesting here is getting a bit wild. Can/should we not combine some of it? Is there a better way? I did kinda just copy what we're already doing for isOpen.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can group all actions together, but for state if we're putting it in one context, there would be no way to subscribe to changes to a single value instead of all of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I guess it is OK then.

{children}
</DrawerActionsContext.Provider>
</DrawerSetCurrentTabContext.Provider>
</DrawerCurrentTabStateContext.Provider>
</DrawerSetOpenStateContext.Provider>
</DrawerOpenStateContext.Provider>
</DrawerStateContext.Provider>
Expand All @@ -152,11 +195,21 @@ const DrawerContextGrabber: React.FunctionComponent = ({ children }) => {
const drawerToolbarContext = useDrawerToolbarContext();
const actions = useContext(DrawerActionsContext);
const openStateSetter = useContext(DrawerSetOpenStateContext);
const currentTabSetter = useContext(DrawerSetCurrentTabContext);
actions.current.openDrawer = drawerToolbarContext.openDrawer;
actions.current.closeDrawer = drawerToolbarContext.closeDrawer;

useEffect(() => {
openStateSetter(drawerToolbarContext.isDrawerOpen);
}, [drawerToolbarContext.isDrawerOpen, openStateSetter]);

useEffect(() => {
const currentTab =
drawerToolbarContext.getActiveDrawerContent()?.id ?? null;

currentTabSetter(currentTab);
}, [drawerToolbarContext, currentTabSetter]);

Comment on lines +200 to +206
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

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

This useEffect will run on every change to drawerToolbarContext, but it should only run when the active drawer content actually changes. Consider extracting drawerToolbarContext.getActiveDrawerContent()?.id to the dependency array or using a useMemo to optimize this effect.

Suggested change
useEffect(() => {
const currentTab =
drawerToolbarContext.getActiveDrawerContent()?.id ?? null;
currentTabSetter(currentTab);
}, [drawerToolbarContext, currentTabSetter]);
const activeDrawerContentId = useMemo(
() => drawerToolbarContext.getActiveDrawerContent()?.id ?? null,
[drawerToolbarContext]
);
useEffect(() => {
currentTabSetter(activeDrawerContentId);
}, [activeDrawerContentId, currentTabSetter]);

Copilot uses AI. Check for mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing we wouldn't want this because we want open/close even if it is the same tab?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't a value we can check. There's a method. 🤷

return <>{children}</>;
};

Expand Down
30 changes: 30 additions & 0 deletions packages/compass-telemetry/src/telemetry-events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1458,6 +1458,34 @@ type IndexDroppedEvent = ConnectionScopedEvent<{
};
}>;

/**
* This event is fired when user opens a drawer section. Either by switching
* to it via the drawer toolbar or by opening the drawer and the first tab is
* this drawer section.
*
* @category Gen AI
*/
type DrawerSectionOpenedEvent = CommonEvent<{
name: 'Drawer Section Opened';
payload: {
sectionId: string;
};
}>;

/**
* This event is fired when user closes a drawer section. Either by switching
* to another tab via the drawer toolbar or by closing the drawer when the
* active tab is this drawer section.
*
* @category Gen AI
*/
type DrawerSectionClosedEvent = CommonEvent<{
name: 'Drawer Section Closed';
payload: {
sectionId: string;
};
}>;

/**
* This event is fired when user enters a prompt in the assistant chat
* and hits "enter".
Expand Down Expand Up @@ -3174,6 +3202,8 @@ export type TelemetryEvent =
| DocumentDeletedEvent
| DocumentInsertedEvent
| DocumentUpdatedEvent
| DrawerSectionOpenedEvent
| DrawerSectionClosedEvent
| EditorTypeChangedEvent
| ErrorFetchingAttributesEvent
| ExplainPlanExecutedEvent
Expand Down
10 changes: 10 additions & 0 deletions packages/compass-web/src/entrypoint.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,16 @@ const CompassWeb = ({
item_label: item.label,
});
}}
onDrawerSectionOpen={(drawerSectionId) => {
onTrackRef.current?.('Drawer Section Opened', {
sectionId: drawerSectionId,
});
}}
onDrawerSectionHide={(drawerSectionId) => {
onTrackRef.current?.('Drawer Section Closed', {
sectionId: drawerSectionId,
});
}}
onSignalMount={(id) => {
onTrackRef.current?.('Signal Shown', { id });
}}
Expand Down
6 changes: 6 additions & 0 deletions packages/compass/src/app/components/home.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,12 @@ export default function ThemedHome(
item_label: item.label,
});
}}
onDrawerSectionOpen={(drawerSectionId) => {
track('Drawer Section Opened', { sectionId: drawerSectionId });
}}
onDrawerSectionHide={(drawerSectionId) => {
track('Drawer Section Closed', { sectionId: drawerSectionId });
}}
utmSource="compass"
utmMedium="product"
onSignalMount={(id) => {
Expand Down
Loading